[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 27/32] nvdimm: support DSM_CMD_IMPLEMENTED fu
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v3 27/32] nvdimm: support DSM_CMD_IMPLEMENTED function |
Date: |
Thu, 15 Oct 2015 17:07:45 +0200 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Wed, Oct 14, 2015 at 10:50:40PM +0800, Xiao Guangrong wrote:
> On 10/14/2015 05:40 PM, Stefan Hajnoczi wrote:
> >On Sun, Oct 11, 2015 at 11:52:59AM +0800, Xiao Guangrong wrote:
> >>+ out = (dsm_out *)in;
> >>+
> >>+ revision = in->arg1;
> >>+ function = in->arg2;
> >>+ handle = in->handle;
> >>+ le32_to_cpus(&revision);
> >>+ le32_to_cpus(&function);
> >>+ le32_to_cpus(&handle);
> >>+
> >>+ nvdebug("UUID " UUID_FMT ".\n", in->arg0[0], in->arg0[1], in->arg0[2],
> >>+ in->arg0[3], in->arg0[4], in->arg0[5], in->arg0[6],
> >>+ in->arg0[7], in->arg0[8], in->arg0[9], in->arg0[10],
> >>+ in->arg0[11], in->arg0[12], in->arg0[13], in->arg0[14],
> >>+ in->arg0[15]);
> >>+ nvdebug("Revision %#x Function %#x Handler %#x.\n", revision, function,
> >>+ handle);
> >>+
> >>+ if (revision != DSM_REVISION) {
> >>+ nvdebug("Revision %#x is not supported, expect %#x.\n",
> >>+ revision, DSM_REVISION);
> >>+ goto exit;
> >>+ }
> >>+
> >>+ if (!handle) {
> >>+ if (!dsm_is_root_uuid(in->arg0)) {
> >
> >Please don't dereference 'in' or pass it to other functions. Avoid race
> >conditions with guest vcpus by coping in the entire dsm_in struct.
> >
> >This is like a system call - the kernel cannot trust userspace memory
> >and must copy in before accessing data. The same rules apply.
> >
>
> It's little different for QEMU:
> - the memory address is always valid to QEMU, it's not always true for Kernel
> due to context-switch
>
> - we have checked the header before use it's data, for example, when we get
> data from GET_NAMESPACE_DATA, we have got the @offset and @length from the
> memory, then copy memory based on these values, that means the userspace
> has no chance to cause buffer overflow by increasing these values at
> runtime.
>
> The scenario for our case is simple but Kernel is difficult to do
> check_all_before_use as many paths may be involved.
>
> - guest changes some data is okay, the worst case is that the label data is
> corrupted. This is caused by guest itself. Kernel also supports this kind
> of behaviour, e,g. network TX zero copy, the userspace page is being
> transferred while userspace can still access it.
>
> - it's 4K size on x86, full copy wastes CPU time too much.
This isn't performance-critical code and I don't want to review it
keeping the race conditions in mind the whole time. Also, if the code
is modified in the future, the chance of introducing a race is high.
I see this as premature optimization, please just copy in data.
Stefan
[Qemu-devel] [PATCH v3 30/32] nvdimm: support DSM_CMD_SET_NAMESPACE_LABEL_DATA, Xiao Guangrong, 2015/10/10
[Qemu-devel] [PATCH v3 31/32] nvdimm: allow using whole backend memory as pmem, Xiao Guangrong, 2015/10/10
[Qemu-devel] [PATCH v3 01/32] acpi: add aml_derefof, Xiao Guangrong, 2015/10/10
[Qemu-devel] [PATCH v3 02/32] acpi: add aml_sizeof, Xiao Guangrong, 2015/10/10
[Qemu-devel] [PATCH v3 17/32] dimm: abstract dimm device from pc-dimm, Xiao Guangrong, 2015/10/10
[Qemu-devel] [PATCH v3 18/32] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region, Xiao Guangrong, 2015/10/10
[Qemu-devel] [PATCH v3 13/32] pc-dimm: make pc_existing_dimms_capacity static and rename it, Xiao Guangrong, 2015/10/10