Re: [Qemu-devel] [PATCH] nvdimm: ensure that dsm memory is read in nvdimm_dsm_write

[Stefan Hajnoczi]

On Tue, Mar 20, 2018 at 07:35:18AM -0400, Artemis Tosini wrote:
> This patch ensures that the client OS does not cause the host to read invalid 
> memory from the NVDIMM DSM. It is not tested, since the Linux NVDIMM driver 
> will not cause an invalid memory read.

Thanks for the patch!

Please move this line into the patch description...

> 
> This patch is for my outreachy assignment, and is my first open source patch.
> 
> From bcb717b761ac62adeda145e895f92e4bde1003af Mon Sep 17 00:00:00 2001
> From: Artemis Tosini <address@hidden>
> Date: Sat, 10 Mar 2018 20:38:07 +0000
> Subject: [PATCH] nvdimm: ensure that dsm memory is read in nvdimm_dsm_write

...here so that git-log(1) will retain it after your patch is merged.

> 
> Signed-off-by: Artemis Tosini <address@hidden>
> ---
>  hw/acpi/nvdimm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 59d6e4254c..67dda723a7 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -838,7 +838,12 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t 
> val, unsigned size)
>       * this by copying DSM memory to QEMU local memory.
>       */
>      in = g_new(NvdimmDsmIn, 1);
> -    cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
> +    if (address_space_read(&address_space_memory, dsm_mem_addr,
> +                          MEMTXATTRS_UNSPECIFIED, in,
> +                          sizeof(*in)) != MEMTX_OK) {
> +                            nvdimm_debug("Failed to read DSM memory");
> +                            goto exit;
> +                          }

Please follow QEMU coding style (see ./CODING_STYLE in the source tree)
with 4 space indentation.  You can use scripts/checkpatch.pl to check
for coding style violations.  Try this:

    if (address_space_read(&address_space_memory, dsm_mem_addr,
                           MEMTXATTRS_UNSPECIFIED, in,
                           sizeof(*in)) != MEMTX_OK) {
        nvdimm_debug("Failed to read DSM memory");
        goto exit;
    }

Please also look at the compiler error that patchew (the continuous
integration system used by QEMU) has reported.

The actual change looks good to me - it stops the device from processing
*in when we were unable to copy bytes from guest RAM.

Stefan

signature.asc
Description: PGP signature