From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:45923)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1ZmMNo-0005Gh-F3
	for qemu-devel@nongnu.org; Wed, 14 Oct 2015 09:46:24 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1ZmMNi-0006vW-QO
	for qemu-devel@nongnu.org; Wed, 14 Oct 2015 09:46:20 -0400
Received: from mail-wi0-x22b.google.com ([2a00:1450:400c:c05::22b]:32999)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1ZmMNi-0006vN-KO
	for qemu-devel@nongnu.org; Wed, 14 Oct 2015 09:46:14 -0400
Received: by wicge5 with SMTP id ge5so102410487wic.0
	for <qemu-devel@nongnu.org>; Wed, 14 Oct 2015 06:46:13 -0700 (PDT)
Date: Wed, 14 Oct 2015 10:40:12 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20151014094012.GB14874@stefanha-thinkpad>
References: <1444535584-18220-1-git-send-email-guangrong.xiao@linux.intel.com>
	<1444535584-18220-28-git-send-email-guangrong.xiao@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1444535584-18220-28-git-send-email-guangrong.xiao@linux.intel.com>
Subject: Re: [Qemu-devel] [PATCH v3 27/32] nvdimm: support
	DSM_CMD_IMPLEMENTED function
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net

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.