Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Tim Deegan <tim@xen.org>
To: Julian Tuminaro <julian.tuminaro@gmail.com>
Cc: wl@xen.org, paul@xen.org, Jenish Rakholiya <rjenish@cmu.edu>,
	ian.jackson@eu.citrix.com,
	Julian Tuminaro <jtuminar@andrew.cmu.edu>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit)
Date: Sun, 10 Nov 2019 13:14:55 +0000
Message-ID: <20191110131455.GB67574@deinos.phlegethon.org> (raw)
In-Reply-To: <20191106022427.9088-1-julian.tuminaro@gmail.com>

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

      parent reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-06  2:24 Julian Tuminaro
2019-11-06 11:03 ` Wei Liu
2019-11-07  9:20 ` Paul Durrant
2019-11-10 13:14 ` Tim Deegan [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191110131455.GB67574@deinos.phlegethon.org \
    --to=tim@xen.org \
    --cc=ian.jackson@eu.citrix.com \
    --cc=jtuminar@andrew.cmu.edu \
    --cc=julian.tuminaro@gmail.com \
    --cc=paul@xen.org \
    --cc=rjenish@cmu.edu \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git