Re: [SeaBIOS] [SEABIOS] [PATCH 1/1] nvme: fix I/O queue length calculation overflow

[Kevin O'Connor]

On Sun, Aug 12, 2018 at 03:42:40PM -0500, Matt DeVillier wrote:
> Commit cd47172 changed the I/O queue length calculation to use the
> Maximum Queue Entries Supported (MQES) value from the capabilities
> register, plus one, with a maximum value of NVME_PAGE_SIZE.
>
> An unintended effect from this is that a MQES value of 0xFFFF yields
> a length of zero, resulting in the queue allocation failing. Fix this
> by checking for a zero length when checking that the length exceeds
> NVME_PAGE_SIZE, and setting to NVME_PAGE_SIZE.
>
> TEST: build/boot on a Purism Librem13v2 with a MyDigitalSSD BPX NVMe
> drive, which reports a MQES of 0xFFFF. Verify NVMe drive present in
> boot menu and OS boots successfully.
> Signed-off-by: Matt DeVillier
>
> src/hw/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/hw/nvme.c b/src/hw/nvme.c
> index e6d739d..c2d6863 100644
> --- a/src/hw/nvme.c
> +++ b/src/hw/nvme.c
> @@ -319,7 +319,7 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct
> nvme_cq *cq, u16 q_idx)
>      int rc;
>      struct nvme_sqe *cmd_create_cq;
>      u16 length = 1 + (ctrl->reg->cap & 0xffff);
> if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
> if (length == 0 || length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
>     length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);

Thanks.  Instead of changing the if statement, could we just change
length here to a u32 to avoid this issue?
>  rc = nvme_init_cq(ctrl, cq, q_idx, length);
>
> @@ -363,7 +363,7 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct
> nvme_sq *sq, u16 q_idx, struct
>      int rc;
>      struct nvme_sqe *cmd_create_sq;
>      u16 length = 1 + (ctrl->reg->cap & 0xffff);
> if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
>
> if (length == 0 || length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
>     length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);
>
> rc = nvme_init_sq(ctrl, sq, q_idx, length, cq);

-Kevin

[SEABIOS] [PATCH 1/1] nvme: fix I/O queue length calculation overflow

[Matt DeVillier]

On Mon, Aug 20, 2018 at 11:16 PM Kevin O'Connor  wrote:
> On Sun, Aug 12, 2018 at 03:42:40PM -0500, Matt DeVillier wrote:
> > Commit cd47172 changed the I/O queue length calculation to use the
> > Maximum Queue Entries Supported (MQES) value from the capabilities
> > register, plus one, with a maximum value of NVME_PAGE_SIZE.
> >
> > An unintended effect from this is that a MQES value of 0xFFFF yields
> > a length of zero, resulting in the queue allocation failing. Fix this
> > by checking for a zero length when checking that the length exceeds
> > NVME_PAGE_SIZE, and setting to NVME_PAGE_SIZE.
> >
> > TEST: build/boot on a Purism Librem13v2 with a MyDigitalSSD BPX NVMe
> > drive, which reports a MQES of 0xFFFF. Verify NVMe drive present in
> > boot menu and OS boots successfully.
> > Signed-off-by: Matt DeVillier
> >
> > src/hw/nvme.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/hw/nvme.c b/src/hw/nvme.c
> > index e6d739d..c2d6863 100644
> > --- a/src/hw/nvme.c
> > +++ b/src/hw/nvme.c
> > @@ -319,7 +319,7 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct
> > nvme_cq *cq, u16 q_idx)
> >      int rc;
> >      struct nvme_sqe *cmd_create_cq;
> >      u16 length = 1 + (ctrl->reg->cap & 0xffff);
> > if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
> > if (length == 0 || length > NVME_PAGE_SIZE / sizeof(struct
>
> nvme_cqe))
> >      length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);
>
> Thanks.  Instead of changing the if statement, could we just change
> length here to a u32 to avoid this issue?

I can't think of why not, I'll test this method and submit an updated
patchset if so - thanks!
> >  rc = nvme_init_cq(ctrl, cq, q_idx, length);
> >
> > @@ -363,7 +363,7 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct
> > nvme_sq *sq, u16 q_idx, struct
> >      int rc;
> >      struct nvme_sqe *cmd_create_sq;
> >      u16 length = 1 + (ctrl->reg->cap & 0xffff);
> > if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
> > if (length == 0 || length > NVME_PAGE_SIZE / sizeof(struct
>
> nvme_cqe))
> >      length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);
> >
> >  rc = nvme_init_sq(ctrl, sq, q_idx, length, cq);
>
> -Kevin