|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit)
Hi,
At 21:24 -0500 on 05 Nov (1572989067), Julian Tuminaro wrote:
> Current implementation of find_os is based on the hard-coded values for
> different Windows version. It uses the value for get the address to
> start looking for DOS header in the given specified range. However, this
> is not scalable to all version of Windows as it will require us to keep
> adding new entries and also due to KASLR, chances of not hitting the PE
> header is significant. We implement a way for 64-bit systems to use IDT
> entry to get a valid exception/interrupt handler and then move back into
> the memory to find the valid DOS header. Since IDT entries are protected
> by PatchGuard, we think our assumption that IDT entries will not be
> corrupted is valid for our purpose. Once we have the image base, we
> search for the DBGKD_GET_VERSION64 structure type in .data section to
> get information required for handshake.
Thank you very much for this - it looks super-useful! Does this
technique only work if the guest kernel has debugging enabled, or can
it work on all systems?
I have some commetns on the code, below.
> /* Windows version details */
> typedef struct {
> uint32_t build;
> @@ -62,6 +64,7 @@ typedef struct {
> uint32_t version; /* +-> NtBuildNumber */
> uint32_t modules; /* +-> PsLoadedModuleList */
> uint32_t prcbs; /* +-> KiProcessorBlock */
> + uint32_t kddl;
This needs a comment describing the Windows name of what it points to.
> +/**
> + * @brief Parse the memory at \a filebase as a valid DOS header and get
> virtual
> + * address offset and size for any given section name (if it exists)
> + *
> + * @param s Pointer to the kdd_state structure
> + * @param filebase Base address of the file structure
> + * @param sectname Pointer to the section name c-string to look for
> + * @param vaddr Pointer to write the virtual address of section start to
> + * (if found)
> + * @param visze Pointer to write the section size to (if found)
> + *
> + * @return -1 on failure to find the section name
> + * @return 0 on success
> + */
> +static int get_pe64_sections(kdd_state *s, uint64_t filebase, char *sectname,
> + uint64_t *vaddr, uint32_t *vsize)
These new functions don't belong in the 'Utility functions' section.
Please move them to beside the other OS-finding code.
> +{
> + uint8_t buf[0x30];
PE_SECT_ENT_SZ, please.
> + uint64_t pe_hdr;
> + uint64_t sect_start;
> + uint16_t num_sections;
> + int ret;
> +
> + ret = -1;
> +
> + if (!s->os.w64) {
> + return ret;
> + }
> +
> + // read PE header offset
> + if (kdd_read_virtual(s, s->cpuid, filebase + DOS_HDR_PE_OFF,
> DOS_HDR_PE_SZ,
> + buf) != DOS_HDR_PE_SZ) {
> + return -1;
> + }
> + pe_hdr = filebase + *(uint32_t *)buf;
Here and elsewhere, please read directly into the variables, e.g.:
uint32_t pe_hdr;
kdd_read_virtual(s, s->cpuid, filebase + DOS_HDR_PE_OFF,
sizeof pe_hdr, &pe_hdr);
pe_hdr += filebase;
That gives neater code and avoids any confusion about the sizes of
various copies and buffers.
> +
> + // read number of sections
> + if (kdd_read_virtual(s, s->cpuid, pe_hdr + PE_NUM_SECTION_OFF,
> + PE_NUM_SECTION_SZ, &buf) != PE_NUM_SECTION_SZ) {
> + return -1;
> + }
> + num_sections = *(uint16_t *)buf;
This needs a check for very large numbers -- loading 65,535 section
headers might take a long time.
> + // read size of optional header
> + if (kdd_read_virtual(s, s->cpuid, pe_hdr + PE_OPT_HDR_SZ_OFF,
> + PE_OPT_HDR_SZ_SZ, &buf) != PE_OPT_HDR_SZ_SZ) {
> + return -1;
> + }
> +
> + // 0x18 is the size of PE header
> + sect_start = pe_hdr + PE_HDR_SZ + *(uint16_t *)buf;
> +
> + for (int i = 0; i < num_sections; i++) {
> + if (kdd_read_virtual(s, s->cpuid, sect_start + (i * PE_SECT_ENT_SZ),
> + PE_SECT_ENT_SZ, &buf) != PE_SECT_ENT_SZ) {
> + return -1;
> + }
> +
> + if (!strncmp(sectname, (char *)(buf + PE_SECT_NAME_OFF),
> + PE_SECT_NAME_SZ)) {
> + *vaddr = filebase + *(uint32_t *)(buf + PE_SECT_RVA_OFF);
> + *vsize = *(uint32_t *)(buf + PE_SECT_VSIZE_OFF);
> + ret = 0;
> + break;
Just 'return 0' will do here, and..
> + }
> + }
> +
> + return ret;
return -1 here, and drop 'ret'.
> +}
> +
> +/**
> + * @brief Get the OS information like base address, minor version,
> + * PsLoadedModuleList and DebuggerDataList (basically the fields of
> + * DBGKD_GET_VERSION64 struture required to do handshake?).
> + *
> + * This is done by reading the IDT entry for divide-by-zero exception and
> + * searching back into the memory for DOS header (which is our kernel base).
> + * Once we have the kernel base, we parse the PE header and look for kernel
> + * base address in the .data section. Once we have possible values, we look
> for
> + * DBGKD_GET_VERSION64 block by using following heuristics on the address
> which
> + * has the kernel base:
> + *
> + * - at address [-0x10], it should have 0xf as the MajorVersion
> + * - at address [+0x8], it should have a valid kernel memory address
> pointing
> + * in .data
> + * - at address [+0x10], it should have a valid kernel memory address
> pointing
> + * in .data
> + *
> + * @param s Pointer to the kdd state
> + */
> +static void get_os_info_64(kdd_state *s)
> +{
> + kdd_ctrl ctrl;
> + int ret;
> + uint64_t buf;
> + uint64_t idt0_addr;
> + uint64_t base;
> + uint64_t caddr;
> + uint64_t data_base;
> + uint32_t data_size;
> + uint64_t modptr;
> + uint64_t kddl;
> + uint16_t minor;
> + uint8_t dbgkd_get_version64[0x28];
DBGKD_GET_VERSION64_SZ, please
> +
> + /* TODO: right now, we are forcing this to 1 (as we only support 64 bit
> + * system, however, we should use kdd_state or hvm calls to check if we
> are
> + * in 64-bit
> + */
> + s->os.w64 = 1;
At the point where you call this, s->os == unknown_os, so you are
modifying unknown_os here!
Please declare another static kdd_os to fill in here. Then you can
set w64=1 in that without needing any TODOs or checks here.
I would also prefer if this function just returned 0/-1 - then
you don't need a 'goto fail' path. You can just return -1
everywhere, and you can call it from the bottom of 'find_os()', as:
if (get_os_info_64(s) != 0)
s->os = unknown_os;
> + // if we are no in 64-bit mode, fail
> + if (!s->os.w64) {
> + goto fail;
> + }
> +
> + // get control registers for our os
> + ret = kdd_get_ctrl(s->guest, s->cpuid, &ctrl, s->os.w64);
> + if (ret) {
> + goto fail;
> + }
> +
> + // read the div-by-zero handler function address
> + kdd_read_virtual(s, s->cpuid, ctrl.c64.idt_base + 8, 8, &buf);
> + idt0_addr = ((uint64_t)buf << 32) & 0xffffffff00000000;
> +
> + kdd_read_virtual(s, s->cpuid, ctrl.c64.idt_base, 8, &buf);
> + idt0_addr |= ((buf >> 32) & 0xffff0000);
> + idt0_addr |= (buf & 0xffff);
> +
> + KDD_LOG(s, "idt0 addr: %p\n", (void *)idt0_addr);
> + printf("idt0 addr: %p\n", (void *)idt0_addr);
Please drop all printf() calls and just use KDD_LOG() / KDD_DEBUG
> + // get the page start and look for "MZ" file header
> +
> + base = idt0_addr & ~(PAGE_SIZE - 1);
> + // printf("%p\n", (void *)base);
Please remove all commented-out code.
> + while (1) {
Please set some reasonable limit to how far this will search.
Presumably we expect to find the header within < 1 GB ?
> + uint16_t val;
> + if (kdd_read_virtual(s, s->cpuid, base, 2, &val) != 2) {
> + // just move going back?? this is bad though
> + printf("ran into unmapped region without finding PE header\n");
KDD_DEBUG() and return false.
> + }
> +
> + if (val == 0x5a4d) { // MZ
> + // printf("maybe success\n");
> + break;
> + }
> +
> + base -= PAGE_SIZE;
> + }
> +
> + KDD_LOG(s, "base: %p\n", (void *)base);
> +
> + // found the data section start
> + if (get_pe64_sections(s, base, ".data", &data_base, &data_size)) {
> + goto fail;
> + }
> +
> + // look for addresses which has kernel base written into it
> + caddr = data_base;
> +
> + modptr = 0;
> + kddl = 0;
> + minor = 0;
> +
> + while (caddr < data_base + data_size) {
This needs a limit: data_size came from guest memory and we don't want
to do 500 million iterations here.
> + if (kdd_read_virtual(s, s->cpuid, caddr, SIZE_PTR64, &buf) !=
> + SIZE_PTR64) {
> + // reached end and found nothing
> + goto fail;
> + }
> +
> + // if we found base in the memory addresses
> + if (buf == base) {
> + // read the DBGKD_GET_VERSION64 struct
> + if (kdd_read_virtual(s, s->cpuid, caddr - DBGKD_KERN_BASE_OFF,
> + DBGKD_GET_VERSION64_SZ, dbgkd_get_version64) ==
> + DBGKD_GET_VERSION64_SZ) {
> + // check if major version is 0xf
> + if (dbgkd_get_version64[0] == '\x0f') {
> +
> + // read minor version, PsLoadedModuleList pointer and
> + // DebuggerDataList
> + modptr =
> + *(uint64_t *)(dbgkd_get_version64 +
> DBGKD_MOD_LIST_OFF);
> + kddl = *(uint64_t *)(dbgkd_get_version64 +
> DBGKD_KDDL_OFF);
> + minor =
> + *(uint16_t *)(dbgkd_get_version64 + DBGKD_MINOR_OFF);
I think this code would be much cleaner if you defined a struct with the
correct fields instead of trying to parse them by hand out of a buffer.
> + // do heuristic check
> + if (modptr && kddl && modptr != kddl && kddl != base &&
> + base != modptr && modptr >= data_base &&
> + modptr < (data_base + data_size) &&
> + kddl >= data_base &&
> + kddl < (data_base + data_size)) {
> + // my_memdump(s, caddr - 0x10, 0x30);
> + break;
> + }
> +
> + }
> + }
> +
> + }
> +
> + caddr += sizeof(void *);
> + }
We need a test here for whether we ran off the end of the while()
loop without finding the magic struct.
> + // TODO: use KDD_LOG?
> + printf("base: %p\n", (void *)base);
> + printf("modules list: %p\n", (void *)modptr);
> + printf("kddl: %p\n", (void *)kddl);
> + printf("minor version: 0x%hx\n", minor);
> +
> + s->os.base = base;
> + s->os.modules = modptr - base;
> + s->os.kddl = kddl - base;
> + s->os.build = (uint32_t) minor;
> + return;
> +
> +fail:
> + // XXX: TODO: handle failure case
> + s->os = unknown_os;
> + return;
> +}
> +
>
>
> /*****************************************************************************
> * How to send packets and acks.
> @@ -534,6 +915,12 @@ static void kdd_handle_handshake(kdd_state *s)
> {
> /* Figure out what we're looking at */
> find_os(s);
> +
> + /* if unknown os, use the idt method */
> + if (!s->os.base) {
> + get_os_info_64(s);
> + }
As I said above, please call get_os_info_64() from find_os() instead
of calling it from here.
> kdd_send_string(s, "[kdd: %s @0x%"PRIx64"]\r\n", s->os.name, s->os.base);
>
> /* Respond with some details about the debugger stub we simulate */
> @@ -555,7 +942,7 @@ static void kdd_handle_handshake(kdd_state *s)
> s->txp.cmd.shake.u3[2] = 0x55;
> s->txp.cmd.shake.kern_addr = s->os.base;
> s->txp.cmd.shake.mods_addr = s->os.base + s->os.modules;
> - s->txp.cmd.shake.data_addr = 0; /* Debugger data probably doesn't exist
> */
> + s->txp.cmd.shake.data_addr = s->os.base + s->os.kddl; // 0; /* Debugger
> data probably doesn't exist */
Please only set this if s->os.kddl is != 0.
As I said, overall this looks like a really useful improvement and I
look forward to seing your revised patch!
Thanks,
Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |