On Tue, 9 Mar 2021 at 11:45, Klaus Jensen <[email protected]> wrote:
>
> From: Klaus Jensen <[email protected]>
>
> ZASL (Zone Append Size Limit) is defined exactly like MDTS (Maximum Data
> Transfer Size), that is, it is a value in units of the minimum memory
> page size (CAP.MPSMIN) and is reported as a power of two.
>
> The 'mdts' nvme device parameter is specified as in the spec, but the
> 'zoned.append_size_limit' parameter is specified in bytes. This is
> suboptimal for a number of reasons:
>
> 1. It is just plain confusing wrt. the definition of mdts.
> 2. There is a lot of complexity involved in validating the value; it
> must be a power of two, it should be larger than 4k, if it is zero
> we set it internally to mdts, but still report it as zero.
> 3. While "hw/block/nvme: improve invalid zasl value reporting"
> slightly improved the handling of the parameter, the validation is
> still wrong; it does not depend on CC.MPS, it depends on
> CAP.MPSMIN. And we are not even checking that it is actually less
> than or equal to MDTS, which is kinda the *one* condition it must
> satisfy.
>
> Fix this by defining zasl exactly like mdts and checking the one thing
> that it must satisfy (that it is less than or equal to mdts). Also,
> change the default value from 128KiB to 0 (aka, whatever mdts is).
> @@ -2144,10 +2142,9 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest
> *req, bool append,
> goto invalid;
> }
>
> - if (nvme_l2b(ns, nlb) > (n->page_size << n->zasl)) {
> - trace_pci_nvme_err_append_too_large(slba, nlb, n->zasl);
> - status = NVME_INVALID_FIELD;
> - goto invalid;
> + if (n->params.zasl && data_size > n->page_size <<
> n->params.zasl) {
> + trace_pci_nvme_err_zasl(data_size);
> + return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> slba = zone->w_ptr;
Hi; Coverity points out a possible overflow here (CID 1450756):
n->page_size is a uint32_t, and n->params.zasl is a uint8_t, so
the "n->page_size << n->params.zasl" will be done as 32-bit arithmetic;
but it is then compared against a uint64_t data_size.
Is this an overflow that can never happen (ie a false positive), or
should the RHS of the comparison be done as 64-bit arithmetic by
adding a cast ?
thanks
-- PMM
