Re: [PULL 05/27] hw/cxl/cxl-mailbox-utils: Media operations Sanitize and Write Zeros commands CXL r3.2(8.2.10.9.5.3)

[Peter Maydell <peter.maydell@linaro.org>]

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 <peter.maydell@linaro.org> wrote:
>
> On Wed, 14 May 2025 at 12:50, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Vinayak Holikatti <vinayak.kh@samsung.com>
> >
> > 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 <vinayak.kh@samsung.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Message-Id: <20250305092501.191929-6-Jonathan.Cameron@huawei.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
>
> > +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