Ping -- these issues in this change seem to still be present in the current version. Would somebody like to have a look at them ?
On Thu, 10 Jul 2025 at 14:23, Peter Maydell <[email protected]> wrote: > > On Wed, 14 May 2025 at 12:50, Michael S. Tsirkin <[email protected]> wrote: > > > > From: Vinayak Holikatti <[email protected]> > > > > CXL spec 3.2 section 8.2.10.9.5.3 describes media operations commands. > > CXL devices supports media operations Sanitize and Write zero command. > > > > Signed-off-by: Vinayak Holikatti <[email protected]> > > Signed-off-by: Jonathan Cameron <[email protected]> > > Message-Id: <[email protected]> > > Reviewed-by: Michael S. Tsirkin <[email protected]> > > Signed-off-by: Michael S. Tsirkin <[email protected]> > > --- > > > +static int validate_dpa_addr(CXLType3Dev *ct3d, uint64_t dpa_addr, > > + size_t length) > > +{ > > + uint64_t vmr_size, pmr_size, dc_size; > > + > > + if ((dpa_addr % CXL_CACHE_LINE_SIZE) || > > + (length % CXL_CACHE_LINE_SIZE) || > > + (length <= 0)) { > > + return -EINVAL; > > + } > > + > > + vmr_size = get_vmr_size(ct3d, NULL); > > + pmr_size = get_pmr_size(ct3d, NULL); > > + dc_size = get_dc_size(ct3d, NULL); > > + > > + if (dpa_addr + length > vmr_size + pmr_size + dc_size) { > > Hi; Coverity flagged up a potential issue in this function (CID 1610093) > Partly it is a false positive (it thinks vmr_size etc can > be -1, but they won't I assume ever be memory regions of that > size), but it did make me notice that this address/length > check looks wrong. If the guest can pass us a (dpa_addr, length) > combination that overflows a 64-bit integer then we can > incorrectly pass this length test. > > > + return -EINVAL; > > + } > > + > > + if (dpa_addr > vmr_size + pmr_size) { > > + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { > > + return -ENODEV; > > + } > > + } > > + > > + return 0; > > +} > > > > > +static CXLRetCode media_operations_sanitize(CXLType3Dev *ct3d, > > + uint8_t *payload_in, > > + size_t len_in, > > + uint8_t *payload_out, > > + size_t *len_out, > > + uint8_t fill_value, > > + CXLCCI *cci) > > +{ > > + struct media_operations_sanitize { > > + uint8_t media_operation_class; > > + uint8_t media_operation_subclass; > > + uint8_t rsvd[2]; > > + uint32_t dpa_range_count; > > + struct dpa_range_list_entry dpa_range_list[]; > > + } QEMU_PACKED *media_op_in_sanitize_pl = (void *)payload_in; > > + uint32_t dpa_range_count = media_op_in_sanitize_pl->dpa_range_count; > > This looks dubious -- a packed struct presumably from the > guest, with a 32-bit value, that we are reading without > doing any handling of host endianness ? > > thanks > -- PMM
