* [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit)
@ 2019-11-06 2:24 Julian Tuminaro
2019-11-06 11:03 ` Wei Liu
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Julian Tuminaro @ 2019-11-06 2:24 UTC (permalink / raw)
To: xen-devel
Cc: wl, paul, ian.jackson, Jenish Rakholiya, tim, Julian Tuminaro,
julian.tuminaro
From: "julian.tuminaro@gmail.com" <julian.tuminaro@gmail.com>
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.
Currently, this is a work in progress feature and current patch only
supports the handshake and memory read/write on 64-bit systems.
Signed-off-by: Jenish Rakholiya <rjenish@cmu.edu>
Signed-off-by: Julian Tuminaro <jtuminar@andrew.cmu.edu>
---
tools/debugger/kdd/kdd.c | 389 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 388 insertions(+), 1 deletion(-)
diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index fb8c645355..407b70a21c 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -51,6 +51,8 @@
#include "kdd.h"
+// TODO: make fix this to actually use address instead of offset?
+// TODO: Maybe clean something as well?
/* 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;
} kdd_os;
/* State of the debugger stub */
@@ -85,6 +88,106 @@ typedef struct {
kdd_os os; /* OS-specific magic numbers */
} kdd_state;
+/**
+ * @brief Size of pointer on 64 machine
+ */
+#define SIZE_PTR64 8
+
+/**
+ * @brief Size of pointer on 32 machine
+ */
+#define SIZE_PTR32 4
+
+
+/*****************************************************************************
+ * PE and DOS Header related offsets
+ */
+
+/**
+ * @brief Offset in DOS header to look for PE header
+ */
+#define DOS_HDR_PE_OFF 0x3c
+
+/**
+ * @brief Size of PE header offset field in DOS header
+ */
+#define DOS_HDR_PE_SZ 4
+
+/**
+ * @brief Offset of number of sections field in PE header
+ */
+#define PE_NUM_SECTION_OFF 0x6
+
+/**
+ * @brief Size of number of sections field in PE header
+ */
+#define PE_NUM_SECTION_SZ 2
+
+/**
+ * @brief Offset of optional header size field in PE header
+ */
+#define PE_OPT_HDR_SZ_OFF 0x14
+
+/**
+ * @brief Size of optional header size field in PE header
+ */
+#define PE_OPT_HDR_SZ_SZ 2
+
+/**
+ * @brief Size of PE header
+ */
+#define PE_HDR_SZ 0x18
+
+/**
+ * @brief Size of section header entries in PE
+ */
+#define PE_SECT_ENT_SZ 0x28
+
+/**
+ * @brief Offset of name field in section header entry
+ */
+#define PE_SECT_NAME_OFF 0
+
+/**
+ * @brief Size of name field in section header entry
+ */
+#define PE_SECT_NAME_SZ 0x8
+
+/**
+ * @brief Offset of virtual address (RVA) field in section header entry
+ */
+#define PE_SECT_RVA_OFF 0xc
+
+/**
+ * @brief Offset of virtual size field in section header entry
+ */
+#define PE_SECT_VSIZE_OFF 0x8
+
+/**
+ * @brief Size of DBGKD_GET_VERSION64 struct
+ */
+#define DBGKD_GET_VERSION64_SZ 0x28
+
+/**
+ * @brief Offset of KERN_BASE in DBGKD_GET_VERSION64 struct
+ */
+#define DBGKD_KERN_BASE_OFF 0x10
+
+/**
+ * @brief Offset of PsLoadedModulesList in DBGKD_GET_VERSION64 struct
+ */
+#define DBGKD_MOD_LIST_OFF 0x18
+
+/**
+ * @brief Offset of DebuggerDataList in DBGKD_GET_VERSION64 struct
+ */
+#define DBGKD_KDDL_OFF 0x20
+
+/**
+ * @brief Offset of DebuggerDataList in DBGKD_GET_VERSION64 struct
+ */
+#define DBGKD_MINOR_OFF 0x2
+
/*****************************************************************************
* Utility functions
*/
@@ -390,6 +493,284 @@ static void find_os(kdd_state *s)
s->os = unknown_os;
}
+#if 0
+/**
+ * @brief Temporary function for printing memory dump while debugging
+ * Dumps in the format of QWORD type
+ *
+ * @param s Pointer to the kdd_state structure
+ * @param addr Address to start dumping memory from
+ * @param size Number of bytes to print (automatically aligned to higher
+ * multiple of 8 bytes
+ *
+ * @note TODO: Remove this before pushing to master
+ * @note TODO: Maybe add level of logging to kdd (using -v option) - The
+ * idea of using printf instead of KDD_LOG was to not print all other unwanted
+ * logging output
+ */
+static void my_memdump(kdd_state *s, uint64_t addr, int size)
+{
+ int ret;
+ uint64_t buf;
+
+ // we don't handle mis-aligned addresses
+ if (addr & 7) {
+ // XXX: TODO: don't do this
+ return;
+ }
+
+ // dump memory 1 QWORD at a time
+ // format: <address> [offset from start]: dump1 dump2
+ for (int i = 0; i < size; i += 16) {
+
+ // read 8 bytes - on failure, break
+ ret = kdd_read_virtual(s, s->cpuid, addr + i, 8, &buf);
+ if (ret != 8) {
+ break;
+ }
+
+ // print first part
+ printf("0x%p [+0x%03x]: %p ", (void *)(addr + i), i, (void *)buf);
+
+ // read next 8 bytes and print it
+ ret = kdd_read_virtual(s, s->cpuid, addr + i + 8, 8, &buf);
+ if (ret != 8) {
+ break;
+ }
+ printf("%p\n", (void *)buf);
+ }
+ printf("\n");
+}
+#endif
+
+/**
+ * @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)
+{
+ uint8_t buf[0x30];
+ 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;
+
+ // 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;
+
+ // 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;
+ }
+ }
+
+ return 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];
+
+ /* 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;
+
+ // 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);
+
+ // get the page start and look for "MZ" file header
+
+ base = idt0_addr & ~(PAGE_SIZE - 1);
+ // printf("%p\n", (void *)base);
+
+ while (1) {
+ 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");
+ }
+
+ 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) {
+ 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);
+
+ // 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 *);
+ }
+
+ // 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);
+ }
+
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 */
KDD_LOG(s, "Client initial handshake: %s\n", s->os.name);
kdd_send_cmd(s, KDD_CMD_SHAKE, 0);
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit)
2019-11-06 2:24 [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit) Julian Tuminaro
@ 2019-11-06 11:03 ` Wei Liu
2019-11-07 9:20 ` Paul Durrant
2019-11-10 13:14 ` Tim Deegan
2 siblings, 0 replies; 4+ messages in thread
From: Wei Liu @ 2019-11-06 11:03 UTC (permalink / raw)
To: Julian Tuminaro
Cc: wl, paul, ian.jackson, Jenish Rakholiya, tim, Julian Tuminaro, xen-devel
Hi Julian
Thanks for your patch.
On Tue, Nov 05, 2019 at 09:24:27PM -0500, Julian Tuminaro wrote:
[...]
>
> +#if 0
> +/**
> + * @brief Temporary function for printing memory dump while debugging
> + * Dumps in the format of QWORD type
> + *
> + * @param s Pointer to the kdd_state structure
> + * @param addr Address to start dumping memory from
> + * @param size Number of bytes to print (automatically aligned to higher
> + * multiple of 8 bytes
> + *
> + * @note TODO: Remove this before pushing to master
:-)
> + * @note TODO: Maybe add level of logging to kdd (using -v option) - The
> + * idea of using printf instead of KDD_LOG was to not print all other unwanted
> + * logging output
> + */
> +static void my_memdump(kdd_state *s, uint64_t addr, int size)
If this function is not used, please leave it out. I generally prefer to
not commit dead code unless absolutely necessary.
Also the code can use a bit of cleanup because it contains a lot of
commented out code which is probably for debugging only.
I'm not familiar with KDD so I will leave the judgement of this patch to
Paul and Tim. Please wait for them to reply before sending out a new
version.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit)
2019-11-06 2:24 [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit) Julian Tuminaro
2019-11-06 11:03 ` Wei Liu
@ 2019-11-07 9:20 ` Paul Durrant
2019-11-10 13:14 ` Tim Deegan
2 siblings, 0 replies; 4+ messages in thread
From: Paul Durrant @ 2019-11-07 9:20 UTC (permalink / raw)
To: Julian Tuminaro
Cc: Wei Liu, Ian Jackson, Jenish Rakholiya, tim, Julian Tuminaro, xen-devel
On Wed, 6 Nov 2019 at 02:24, Julian Tuminaro <julian.tuminaro@gmail.com> wrote:
>
> From: "julian.tuminaro@gmail.com" <julian.tuminaro@gmail.com>
>
>
> 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.
>
> Currently, this is a work in progress feature and current patch only
> supports the handshake and memory read/write on 64-bit systems.
>
> Signed-off-by: Jenish Rakholiya <rjenish@cmu.edu>
> Signed-off-by: Julian Tuminaro <jtuminar@andrew.cmu.edu>
Julian,
Thanks for the patch. This work is great progress for kdd. Comments
below... Mainly style-related but I think there's also some clean-up
needed and a few tweaks.
> ---
> tools/debugger/kdd/kdd.c | 389 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 388 insertions(+), 1 deletion(-)
>
> diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
> index fb8c645355..407b70a21c 100644
> --- a/tools/debugger/kdd/kdd.c
> +++ b/tools/debugger/kdd/kdd.c
> @@ -51,6 +51,8 @@
>
> #include "kdd.h"
>
> +// TODO: make fix this to actually use address instead of offset?
> +// TODO: Maybe clean something as well?
These TODOs are a not really well enough described to be committed
IMO. I'm totally fine with having TODO or FIXME comments, but they
ought to be clear enough for someone else picking up the code.
> /* 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;
Comment for this field perhaps?
> } kdd_os;
>
> /* State of the debugger stub */
> @@ -85,6 +88,106 @@ typedef struct {
> kdd_os os; /* OS-specific magic numbers */
> } kdd_state;
>
> +/**
> + * @brief Size of pointer on 64 machine
> + */
> +#define SIZE_PTR64 8
> +
> +/**
> + * @brief Size of pointer on 32 machine
> + */
> +#define SIZE_PTR32 4
> +
> +
> +/*****************************************************************************
> + * PE and DOS Header related offsets
> + */
> +
> +/**
> + * @brief Offset in DOS header to look for PE header
> + */
> +#define DOS_HDR_PE_OFF 0x3c
> +
> +/**
> + * @brief Size of PE header offset field in DOS header
> + */
> +#define DOS_HDR_PE_SZ 4
> +
> +/**
> + * @brief Offset of number of sections field in PE header
> + */
> +#define PE_NUM_SECTION_OFF 0x6
> +
> +/**
> + * @brief Size of number of sections field in PE header
> + */
> +#define PE_NUM_SECTION_SZ 2
> +
> +/**
> + * @brief Offset of optional header size field in PE header
> + */
> +#define PE_OPT_HDR_SZ_OFF 0x14
> +
> +/**
> + * @brief Size of optional header size field in PE header
> + */
> +#define PE_OPT_HDR_SZ_SZ 2
> +
> +/**
> + * @brief Size of PE header
> + */
> +#define PE_HDR_SZ 0x18
> +
> +/**
> + * @brief Size of section header entries in PE
> + */
> +#define PE_SECT_ENT_SZ 0x28
> +
> +/**
> + * @brief Offset of name field in section header entry
> + */
> +#define PE_SECT_NAME_OFF 0
> +
> +/**
> + * @brief Size of name field in section header entry
> + */
> +#define PE_SECT_NAME_SZ 0x8
> +
> +/**
> + * @brief Offset of virtual address (RVA) field in section header entry
> + */
> +#define PE_SECT_RVA_OFF 0xc
> +
> +/**
> + * @brief Offset of virtual size field in section header entry
> + */
> +#define PE_SECT_VSIZE_OFF 0x8
> +
> +/**
> + * @brief Size of DBGKD_GET_VERSION64 struct
> + */
> +#define DBGKD_GET_VERSION64_SZ 0x28
> +
> +/**
> + * @brief Offset of KERN_BASE in DBGKD_GET_VERSION64 struct
> + */
> +#define DBGKD_KERN_BASE_OFF 0x10
> +
> +/**
> + * @brief Offset of PsLoadedModulesList in DBGKD_GET_VERSION64 struct
> + */
> +#define DBGKD_MOD_LIST_OFF 0x18
> +
> +/**
> + * @brief Offset of DebuggerDataList in DBGKD_GET_VERSION64 struct
> + */
> +#define DBGKD_KDDL_OFF 0x20
> +
> +/**
> + * @brief Offset of DebuggerDataList in DBGKD_GET_VERSION64 struct
> + */
> +#define DBGKD_MINOR_OFF 0x2
> +
> /*****************************************************************************
> * Utility functions
> */
> @@ -390,6 +493,284 @@ static void find_os(kdd_state *s)
> s->os = unknown_os;
> }
>
> +#if 0
> +/**
> + * @brief Temporary function for printing memory dump while debugging
> + * Dumps in the format of QWORD type
> + *
> + * @param s Pointer to the kdd_state structure
> + * @param addr Address to start dumping memory from
> + * @param size Number of bytes to print (automatically aligned to higher
> + * multiple of 8 bytes
> + *
> + * @note TODO: Remove this before pushing to master
I guess this means you want to leave this out of the actual commit for now?
> + * @note TODO: Maybe add level of logging to kdd (using -v option) - The
> + * idea of using printf instead of KDD_LOG was to not print all other unwanted
> + * logging output
> + */
> +static void my_memdump(kdd_state *s, uint64_t addr, int size)
> +{
> + int ret;
> + uint64_t buf;
> +
> + // we don't handle mis-aligned addresses
General style is not to use // comments, but stick to /* */.
> + if (addr & 7) {
> + // XXX: TODO: don't do this
> + return;
> + }
No requirement for braces around single line clauses.
> +
> + // dump memory 1 QWORD at a time
> + // format: <address> [offset from start]: dump1 dump2
> + for (int i = 0; i < size; i += 16) {
> +
> + // read 8 bytes - on failure, break
> + ret = kdd_read_virtual(s, s->cpuid, addr + i, 8, &buf);
> + if (ret != 8) {
> + break;
> + }
> +
> + // print first part
> + printf("0x%p [+0x%03x]: %p ", (void *)(addr + i), i, (void *)buf);
> +
> + // read next 8 bytes and print it
> + ret = kdd_read_virtual(s, s->cpuid, addr + i + 8, 8, &buf);
> + if (ret != 8) {
> + break;
> + }
> + printf("%p\n", (void *)buf);
> + }
> + printf("\n");
> +}
> +#endif
> +
> +/**
> + * @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)
> +{
> + uint8_t buf[0x30];
> + uint64_t pe_hdr;
> + uint64_t sect_start;
> + uint16_t num_sections;
> + int ret;
> +
> + ret = -1;
You could assign during declaration.
> +
> + if (!s->os.w64) {
> + return ret;
return -1, for consistency, perhaps?
> + }
> +
> + // 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;
> +
> + // 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;
> +
> + // 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;
> + }
> + }
> +
> + return 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];
> +
> + /* 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;
> +
> + // 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);
> +
> + // get the page start and look for "MZ" file header
> +
> + base = idt0_addr & ~(PAGE_SIZE - 1);
> + // printf("%p\n", (void *)base);
> +
> + while (1) {
> + 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");
Bail out of the loop here, or continue? Checking val after a failure
doesn't seem right.
> + }
> +
> + if (val == 0x5a4d) { // MZ
Perhaps #define this value.
> + // 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) {
> + 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);
> +
> + // 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 *);
> + }
> +
> + // TODO: use KDD_LOG?
Looks like this should be fixed.
> + 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
handle in what way?
> + s->os = unknown_os;
No need for a return here.
> + 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);
> + }
> +
> 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 */
That comment doesn't sound right any more.
Thanks,
Paul
>
> KDD_LOG(s, "Client initial handshake: %s\n", s->os.name);
> kdd_send_cmd(s, KDD_CMD_SHAKE, 0);
> --
> 2.17.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit)
2019-11-06 2:24 [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit) Julian Tuminaro
2019-11-06 11:03 ` Wei Liu
2019-11-07 9:20 ` Paul Durrant
@ 2019-11-10 13:14 ` Tim Deegan
2 siblings, 0 replies; 4+ messages in thread
From: Tim Deegan @ 2019-11-10 13:14 UTC (permalink / raw)
To: Julian Tuminaro
Cc: wl, paul, Jenish Rakholiya, ian.jackson, Julian Tuminaro, xen-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-11-10 13:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 2:24 [Xen-devel] [PATCH 1/1] kdd.c: Add support for initial handshake in KD protocol for Win 7, 8 and 10 (64 bit) Julian Tuminaro
2019-11-06 11:03 ` Wei Liu
2019-11-07 9:20 ` Paul Durrant
2019-11-10 13:14 ` Tim Deegan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).