On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote:
On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
Can't guest trigger this?
If yes, don't put such code in production please:
this will fill up disk on the host.
Okay, the evil guest can read the IO port freely. I will use nvdimm_debug()
instead.
static void
nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
{
+ NvdimmDsmIn *in;
+ GArray *out;
+ uint32_t buf_size;
+ hwaddr dsm_mem_addr = val;
+
+ nvdimm_debug("dsm memory address %#lx.\n", dsm_mem_addr);
+
+ /*
+ * The DSM memory is mapped to guest address space so an evil guest
+ * can change its content while we are doing DSM emulation. Avoid
+ * this by copying DSM memory to QEMU local memory.
+ */
+ in = g_malloc(TARGET_PAGE_SIZE);
ugh. manual memory management :(
Hmm... Or use GArray? But it is :)
+ cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
is there a requirement address is aligned?
if not this might cross page and crash qemu.
better read just what you need.
Yes, this memory is allocated by BIOS and we asked it to align the memory
with PAGE_SIZE:
bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
false /* high memory */);
+
+ le32_to_cpus(&in->revision);
+ le32_to_cpus(&in->function);
+ le32_to_cpus(&in->handle);
+
+ nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
+ in->handle, in->function);
+
+ out = g_array_new(false, true /* clear */, 1);
export build_alloc_array then, and reuse?
It is good to me, but as your suggestions, this code will be removed.
+
+ /*
+ * function 0 is called to inquire what functions are supported by
+ * OSPM
+ */
+ if (in->function == 0) {
+ build_append_int_noprefix(out, 0 /* No function Supported */,
+ sizeof(uint8_t));
What does this mean? Same comment here and below ...
If its the function 0, we return 0 that indicates no command is supported yet.
Other wise, it is a command request from a evil guest regardless of the result
returned by function 0, we return the status code 1 to indicates this command
is not supported.
+ } else {
+ /* No function is supported yet. */
+ build_append_int_noprefix(out, 1 /* Not Supported */,
+ sizeof(uint8_t));
+ }
+
+ buf_size = cpu_to_le32(out->len);
+ cpu_physical_memory_write(dsm_mem_addr, &buf_size, sizeof(buf_size));
is there a race here?
can guest read this before data is written?
I think no.
It is the SERIALIZED DSM so there is no race in guest. And the CPU has exited
from guest mode when we fill the buffer in the same CPU-context so the guest
can not read the buffer at this point also memory-barrier is not needed here.
+ cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
+ out->len);
What is this doing?
Is this actually writing AML bytecode into guest memory?
The layout of result written into the buffer is like this:
struct NvdimmDsmOut {
/* the size of buffer filled by QEMU. */
uint32_t len;
uint8_t data[0];
} QEMU_PACKED;
typedef struct NvdimmDsmOut NvdimmDsmOut;
So the first cpu_physical_memory_write() writes the @len and the second one you
pointed out writes the real payload.
So either write a function that gets parameters and formats
buffer, or use a structure to do this.
Do not open-code formatting and don't mess with
offsets.
E.g.
struct NvdimmDsmFunc0Out {
/* the size of buffer filled by QEMU. */
uint32_t len;
uint8_t supported;
} QEMU_PACKED;
typedef struct NvdimmDsmFunc0Out NvdimmDsmFunc0Out;
And now
NvdimmDsmFunc0Out func0 = { .len = cpu_to_le32(sizeof(func0)); suppported =
func == 0; };
cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0);
Or if you really insist on using GArray:
build_dsm_out_func0(int function...)
{
uint32_t len;
uint8_t result;
len = sizeof result;
if (function == 0) {
result = 0 /* No function Supported */;
} else {
/* No function is supported yet. */
result = 1 /* Not Supported */;
}
build_append_int_noprefix(out, len, sizeof len);
build_append_int_noprefix(out, result, sizeof result);
assert(out->len < PAGE_SIZE); - is this right?
cpu_physical_memory_write(dsm_mem_addr, out->data,
out->len);
}
but I prefer the former ...
Okay, i prefer the former too ;).
Thank you, Michael!
