From: David Hildenbrand Date: Wed, 6 Sep 2023 12:04:53 +0000 (+0200) Subject: nvdimm: Reject writing label data to ROM instead of crashing QEMU X-Git-Tag: v8.2.0~167^2~11 X-Git-Url: https://git.proxmox.com/?a=commitdiff_plain;h=3a1258399bdf4d4412cbfde36d0d94965eec87b6;p=mirror_qemu.git nvdimm: Reject writing label data to ROM instead of crashing QEMU Currently, when using a true R/O NVDIMM (ROM memory backend) with a label area, the VM can easily crash QEMU by trying to write to the label area, because the ROM memory is mmap'ed without PROT_WRITE. [root@vm-0 ~]# ndctl disable-region region0 disabled 1 region [root@vm-0 ~]# ndctl zero-labels nmem0 -> QEMU segfaults Let's remember whether we have a ROM memory backend and properly reject the write request: [root@vm-0 ~]# ndctl disable-region region0 disabled 1 region [root@vm-0 ~]# ndctl zero-labels nmem0 zeroed 0 nmem In comparison, on a system with a R/W NVDIMM: [root@vm-0 ~]# ndctl disable-region region0 disabled 1 region [root@vm-0 ~]# ndctl zero-labels nmem0 zeroed 1 nmem For ACPI, just return "unsupported", like if no label exists. For spapr, return "H_P2", similar to when no label area exists. Could we rely on the "unarmed" property? Maybe, but it looks cleaner to only disallow what certainly cannot work. After all "unarmed=on" primarily means: cannot accept persistent writes. In theory, there might be setups where devices with "unarmed=on" set could be used to host non-persistent data (temporary files, system RAM, ...); for example, in Linux, admins can overwrite the "readonly" setting and still write to the device -- which will work as long as we're not using ROM. Allowing writing label data in such configurations can make sense. Message-ID: <20230906120503.359863-2-david@redhat.com> Fixes: dbd730e85987 ("nvdimm: check -object memory-backend-file, readonly=on option") Reviewed-by: Stefan Hajnoczi Signed-off-by: David Hildenbrand --- diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index a3b25a92f3..3cbd41629d 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -670,7 +670,8 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr) } static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm, - uint32_t offset, uint32_t length) + uint32_t offset, uint32_t length, + bool is_write) { uint32_t ret = NVDIMM_DSM_RET_STATUS_INVALID; @@ -690,6 +691,10 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm, return ret; } + if (is_write && nvdimm->readonly) { + return NVDIMM_DSM_RET_STATUS_UNSUPPORT; + } + return NVDIMM_DSM_RET_STATUS_SUCCESS; } @@ -713,7 +718,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, get_label_data->length); status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset, - get_label_data->length); + get_label_data->length, false); if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) { nvdimm_dsm_no_payload(status, dsm_mem_addr); return; @@ -752,7 +757,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, set_label_data->length); status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset, - set_label_data->length); + set_label_data->length, true); if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) { nvdimm_dsm_no_payload(status, dsm_mem_addr); return; diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c index 31080c22c9..1631a7d13f 100644 --- a/hw/mem/nvdimm.c +++ b/hw/mem/nvdimm.c @@ -154,6 +154,9 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp) object_get_canonical_path_component(OBJECT(hostmem))); return; } + if (memory_region_is_rom(mr)) { + nvdimm->readonly = true; + } nvdimm->nvdimm_mr = g_new(MemoryRegion, 1); memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm), @@ -207,15 +210,16 @@ static void nvdimm_unrealize(PCDIMMDevice *dimm) * label read/write functions. */ static void nvdimm_validate_rw_label_data(NVDIMMDevice *nvdimm, uint64_t size, - uint64_t offset) + uint64_t offset, bool is_write) { assert((nvdimm->label_size >= size + offset) && (offset + size > offset)); + assert(!is_write || !nvdimm->readonly); } static void nvdimm_read_label_data(NVDIMMDevice *nvdimm, void *buf, uint64_t size, uint64_t offset) { - nvdimm_validate_rw_label_data(nvdimm, size, offset); + nvdimm_validate_rw_label_data(nvdimm, size, offset, false); memcpy(buf, nvdimm->label_data + offset, size); } @@ -229,7 +233,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf, "pmem", NULL); uint64_t backend_offset; - nvdimm_validate_rw_label_data(nvdimm, size, offset); + nvdimm_validate_rw_label_data(nvdimm, size, offset, true); if (!is_pmem) { memcpy(nvdimm->label_data + offset, buf, size); diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c index a8688243a6..60d6d0acc0 100644 --- a/hw/ppc/spapr_nvdimm.c +++ b/hw/ppc/spapr_nvdimm.c @@ -320,7 +320,8 @@ static target_ulong h_scm_write_metadata(PowerPCCPU *cpu, nvdimm = NVDIMM(drc->dev); if ((offset + len < offset) || - (nvdimm->label_size < len + offset)) { + (nvdimm->label_size < len + offset) || + nvdimm->readonly) { return H_P2; } diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h index acf887c83d..d3b763453a 100644 --- a/include/hw/mem/nvdimm.h +++ b/include/hw/mem/nvdimm.h @@ -77,6 +77,12 @@ struct NVDIMMDevice { */ bool unarmed; + /* + * Whether our DIMM is backed by ROM, and even label data cannot be + * written. If set, implies that "unarmed" is also set. + */ + bool readonly; + /* * The PPC64 - spapr requires each nvdimm device have a uuid. */