diff mbox

nvdimm: allow read/write zero-size namespace label

Message ID 5878435d.54b31c0a.39a7b.4e93@mx.google.com
State New
Headers show

Commit Message

Li Qiang Jan. 13, 2017, 3:02 a.m. UTC 
From: Li Qiang <liqiang6-s@360.cn>

The spec doesn't say the namespace label can't be zero
when read/write it. As this is no harmful, just allow
it.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/mem/nvdimm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Xiao Guangrong Jan. 13, 2017, 9 a.m. UTC | #1 
On 01/13/2017 11:02 AM, Li Qiang wrote:
> From: Li Qiang <liqiang6-s@360.cn>
>
> The spec doesn't say the namespace label can't be zero
> when read/write it. As this is no harmful, just allow
> it.
>

WHY?

The spec said that the label should be at least 128K.
Li Qiang Jan. 14, 2017, 11:22 a.m. UTC | #2 
Hello Guangrong,


2017-01-13 17:00 GMT+08:00 Xiao Guangrong <guangrong.xiao@linux.intel.com>:

>
>
> On 01/13/2017 11:02 AM, Li Qiang wrote:
>
>> From: Li Qiang <liqiang6-s@360.cn>
>>
>> The spec doesn't say the namespace label can't be zero
>> when read/write it. As this is no harmful, just allow
>> it.
>>
>>
> WHY?
>
> The spec said that the label should be at least 128K.
>

Yes, the label size has a limit, but in NVDIMM_DSM_Interface_Example.pdf
section 4.5.1
When the guest get namespace label data, the 'Length' is not limited, if it
is 0, it will trigger
this assert.

static void nvdimm_validate_rw_label_data(NVDIMMDevice *nvdimm, uint64_t
size,
                                        uint64_t offset)
{
    assert((nvdimm->label_size >= size + offset) && (offset + size >
offset));
}

Though I don't know what the exact behavior of this action in real
hardware. I just think it should not
trigger assert and exit when the guest get 0-size label data.

Anyway, this is just a suggestion, If my understand is wrong, just ignore
this.

Thanks.
Stefan Hajnoczi Jan. 16, 2017, 1:34 p.m. UTC | #3 
On Sat, Jan 14, 2017 at 07:22:28PM +0800, Li Qiang wrote:
> Hello Guangrong,
> 
> 
> 2017-01-13 17:00 GMT+08:00 Xiao Guangrong <guangrong.xiao@linux.intel.com>:
> 
> >
> >
> > On 01/13/2017 11:02 AM, Li Qiang wrote:
> >
> >> From: Li Qiang <liqiang6-s@360.cn>
> >>
> >> The spec doesn't say the namespace label can't be zero
> >> when read/write it. As this is no harmful, just allow
> >> it.
> >>
> >>
> > WHY?
> >
> > The spec said that the label should be at least 128K.
> >
> 
> Yes, the label size has a limit, but in NVDIMM_DSM_Interface_Example.pdf
> section 4.5.1
> When the guest get namespace label data, the 'Length' is not limited, if it
> is 0, it will trigger
> this assert.
> 
> static void nvdimm_validate_rw_label_data(NVDIMMDevice *nvdimm, uint64_t
> size,
>                                         uint64_t offset)
> {
>     assert((nvdimm->label_size >= size + offset) && (offset + size >
> offset));
> }
> 
> Though I don't know what the exact behavior of this action in real
> hardware. I just think it should not
> trigger assert and exit when the guest get 0-size label data.
> 
> Anyway, this is just a suggestion, If my understand is wrong, just ignore
> this.

QEMU must prevent guests from triggering assertions.  If the assertion
causes a core dump then host resources are consumed and this could be a
denial-of-service.  An assertion failure in nested virtualization can
kill sibling VMs and is therefore also a denial-of-service.

The size=0 case must be handled in some way (either an error or a nop).

Stefan
diff mbox

Patch

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index db896b0..4042097 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -114,7 +114,7 @@  static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
 static void nvdimm_validate_rw_label_data(NVDIMMDevice *nvdimm, uint64_t size,
                                         uint64_t offset)
 {
-    assert((nvdimm->label_size >= size + offset) && (offset + size > offset));
+    assert((nvdimm->label_size >= size + offset) && (offset + size >= offset));
 }
 
 static void nvdimm_read_label_data(NVDIMMDevice *nvdimm, void *buf,