| Message ID | 20240819120317.12505-1-engguopeng@buaa.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] hw/cxl: fix the determination of illegal physical addresses | expand |
On Mon, 19 Aug 2024 20:03:17 +0800 peng guo <engguopeng@buaa.edu.cn> wrote: > When physical address range in the input payload of scan media command > exceeds static_mem_size but does not exceed the sum of static and dynamic > memory, the scan media mailbox command unexpectedly returns an error code > which is CXL_MBOX_INVALID_PA. > > This patch determines whether the physical address is valid in two cases. > If dynamic memory exists, check whether the address range of the request > exceeds the range of static memory and dynamic memory.If dynamic memory > does not exist, then check whether the address range of the request > exceeds the static memory size. > > Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support") Is that the right one, this code is affecting cmd_media_scan_media() not the capabilities one which always limits to static_mem_size and hence also looks wrong. > Signed-off-by: peng guo <engguopeng@buaa.edu.cn> As with the other patch, this needs to go to qemu-devel list + both should have gone to Davidlohr as author the patch you are fixing (sort of it, it's mostly down to what order patches landed in I think). Fan, Davidlohr, do we want to just cover the DCD regions as well with all the scan_media commands? > --- > hw/cxl/cxl-mailbox-utils.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index 3ebbd32e10..b23c6b9b0b 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd, > } > query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE; > > - if (query_start + query_length > cxl_dstate->static_mem_size) { > - return CXL_MBOX_INVALID_PA; > - } > - if (ct3d->dc.num_regions && query_start + query_length >= > + if (ct3d->dc.num_regions) { > + if (query_start + query_length >= > cxl_dstate->static_mem_size + ct3d->dc.total_capacity) { > + return CXL_MBOX_INVALID_PA; > + } > + } else if (query_start + query_length > cxl_dstate->static_mem_size) { > return CXL_MBOX_INVALID_PA; > } Can we not rely on dc.total_capacity == 0 if num_regions == 0/ >
On Fri, 23 Aug 2024, Jonathan Cameron wrote:\n >On Mon, 19 Aug 2024 20:03:17 +0800 >peng guo <engguopeng@buaa.edu.cn> wrote: > >> When physical address range in the input payload of scan media command >> exceeds static_mem_size but does not exceed the sum of static and dynamic >> memory, the scan media mailbox command unexpectedly returns an error code >> which is CXL_MBOX_INVALID_PA. >> >> This patch determines whether the physical address is valid in two cases. >> If dynamic memory exists, check whether the address range of the request >> exceeds the range of static memory and dynamic memory.If dynamic memory ^ space for a new sentence >> does not exist, then check whether the address range of the request >> exceeds the static memory size. >> >> Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support") >Is that the right one, this code is affecting cmd_media_scan_media() >not the capabilities one which always limits to static_mem_size and >hence also looks wrong. Yeah it is the right one - both commands were merged together (and yes, they both need to be updated for dcd). Maybe have a helper to consolidate all cases where such ranges are used? uint64_t ct3d_get_total_size(CXLDeviceState *cxl_dstate, CXLType3Dev *ct3d) { return cxl_dstate->static_mem_size + ct3d->dc.total_capacity; } > >> Signed-off-by: peng guo <engguopeng@buaa.edu.cn> > >As with the other patch, this needs to go to qemu-devel list >+ both should have gone to Davidlohr as author the patch you >are fixing (sort of it, it's mostly down to what order patches >landed in I think). > >Fan, Davidlohr, do we want to just cover the DCD regions as >well with all the scan_media commands? Yeah, I think so - particularly since poison management already considers dcd for CXL_MBOX_INVALID_PA. Thanks, Davidlohr > > >> --- >> hw/cxl/cxl-mailbox-utils.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c >> index 3ebbd32e10..b23c6b9b0b 100644 >> --- a/hw/cxl/cxl-mailbox-utils.c >> +++ b/hw/cxl/cxl-mailbox-utils.c >> @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd, >> } >> query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE; >> >> - if (query_start + query_length > cxl_dstate->static_mem_size) { >> - return CXL_MBOX_INVALID_PA; >> - } >> - if (ct3d->dc.num_regions && query_start + query_length >= >> + if (ct3d->dc.num_regions) { >> + if (query_start + query_length >= >> cxl_dstate->static_mem_size + ct3d->dc.total_capacity) { >> + return CXL_MBOX_INVALID_PA; >> + } >> + } else if (query_start + query_length > cxl_dstate->static_mem_size) { >> return CXL_MBOX_INVALID_PA; >> } >Can we not rely on dc.total_capacity == 0 if num_regions == 0/ > >> >
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 3ebbd32e10..b23c6b9b0b 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd, } query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE; - if (query_start + query_length > cxl_dstate->static_mem_size) { - return CXL_MBOX_INVALID_PA; - } - if (ct3d->dc.num_regions && query_start + query_length >= + if (ct3d->dc.num_regions) { + if (query_start + query_length >= cxl_dstate->static_mem_size + ct3d->dc.total_capacity) { + return CXL_MBOX_INVALID_PA; + } + } else if (query_start + query_length > cxl_dstate->static_mem_size) { return CXL_MBOX_INVALID_PA; }
When physical address range in the input payload of scan media command exceeds static_mem_size but does not exceed the sum of static and dynamic memory, the scan media mailbox command unexpectedly returns an error code which is CXL_MBOX_INVALID_PA. This patch determines whether the physical address is valid in two cases. If dynamic memory exists, check whether the address range of the request exceeds the range of static memory and dynamic memory.If dynamic memory does not exist, then check whether the address range of the request exceeds the static memory size. Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support") Signed-off-by: peng guo <engguopeng@buaa.edu.cn> --- hw/cxl/cxl-mailbox-utils.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)