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.
Fixes: dbd730e85987 ("nvdimm: check -object memory-backend-file, readonly=on option")
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/acpi/nvdimm.c | 11 ++++++++---
hw/mem/nvdimm.c | 10 +++++++---
hw/ppc/spapr_nvdimm.c | 3 ++-
include/hw/mem/nvdimm.h | 6 ++++++
4 files changed, 23 insertions(+), 7 deletions(-)
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.
*/
--
2.41.0