Re: [Qemu-devel] [PATCH v3 27/32] nvdimm: support DSM_CMD_IMPLEMENTED function

[Stefan Hajnoczi]

On Sun, Oct 11, 2015 at 11:52:59AM +0800, Xiao Guangrong wrote:
>  static void dsm_write(void *opaque, hwaddr addr,
>                        uint64_t val, unsigned size)
>  {
> +    NVDIMMState *state = opaque;
> +    MemoryRegion *dsm_ram_mr;
> +    dsm_in *in;
> +    dsm_out *out;
> +    uint32_t revision, function, handle;
> +
>      if (val != NOTIFY_VALUE) {
>          fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64, val);
>      }
> +
> +    dsm_ram_mr = memory_region_find(&state->mr, state->page_size,
> +                                    state->page_size).mr;
> +    memory_region_unref(dsm_ram_mr);
> +    in = memory_region_get_ram_ptr(dsm_ram_mr);

This looks suspicious.  Shouldn't the memory_region_unref(dsm_ram_mr)
happen after we're done using it?

> +    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.