Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Minwoo Im]

On 21-04-09 14:36:19, Klaus Jensen wrote:
> On Apr  9 21:31, Minwoo Im wrote:
> > On 21-04-09 13:55:01, Klaus Jensen wrote:
> > > On Apr  9 20:05, Minwoo Im wrote:
> > > > On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
> > > > > NSZE is the total size of the namespace in logical blocks. So the max
> > > > > addressable logical block is NLB minus 1. So your starting logical
> > > > > block is equal to NSZE it is a out of range.
> > > > >
> > > > > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > > > > ---
> > > > >  hw/block/nvme.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > > > index 953ec64729..be9edb1158 100644
> > > > > --- a/hw/block/nvme.c
> > > > > +++ b/hw/block/nvme.c
> > > > > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, 
> > > > > NvmeRequest *req)
> > > > >              uint64_t slba = le64_to_cpu(range[i].slba);
> > > > >              uint32_t nlb = le32_to_cpu(range[i].nlb);
> > > > >
> > > > > -            if (nvme_check_bounds(ns, slba, nlb)) {
> > > > > +            if (nvme_check_bounds(ns, slba, nlb) || slba == 
> > > > > ns->id_ns.nsze) {
> > > >
> > > > This patch also looks like check the boundary about slba.  Should it be
> > > > also checked inside of nvme_check_bounds() ?
> > > 
> > > The catch here is that DSM is like the only command where the number of
> > > logical blocks is a 1s-based value. Otherwise we always have nlb > 0, 
> > > which
> > > means that nvme_check_bounds() will always "do the right thing".
> > > 
> > > My main gripe here is that (in my mind), by definition, a "zero length
> > > range" does not reference any LBAs at all. So how can it result in LBA Out
> > > of Range?
> > 
> > Even if this is not the LBA out of range case which is currently what
> > nvme_check_bounds() checking, but I thought the function checks the
> > bounds so that we can add one more check inside of that function like:
> > (If SLBA is 0-based or not, slba should not be nsze, isn't it ?)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 7244534a89e9..25a7db5ecbd8 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1415,6 +1415,10 @@ static inline uint16_t 
> > nvme_check_bounds(NvmeNamespace *ns, uint64_t slba,
> > {
> >     uint64_t nsze = le64_to_cpu(ns->id_ns.nsze);
> > 
> > +    if (slba == nsze) {
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> > +    }
> > +
> >     if (unlikely(UINT64_MAX - slba < nlb || slba + nlb > nsze)) {
> >         return NVME_LBA_RANGE | NVME_DNR;
> >     }
> > 
> > Or am I missing something here ;) ?
> 
> No, not at all, it's just that this additional check is never needed for any
> other command than DSM since, as far as I remember, DSM is the only command
> with the 1s-based NLB value fuckup.
> 
> This means that nlb will always be at least 1, so slba + 1 > nsze will be
> false if slba == nsze.

Understood :)

Please have:

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>