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
