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

[Matt DeVillier]

>From 2741d96d07855c3bc904d6ce47c82391039bca87 Mon Sep 17 00:00:00 2001
From: Matt DeVillier <matt.devillier at puri.sm>
Date: Tues, 21 Aug 2018 10:00:53 -0500
Subject: [PATCH v2 1/1] nvme: fix I/O queue length calculation overflow

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 due to length being an unsigned
16-bit int, a MQES value of 0xFFFF yields a length of zero, resulting
in the queue allocation failing. Fix this by changing length to a u32.

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 <matt.devillier at puri.sm>
---
 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..2e3aa38 100644
--- a/src/hw/nvme.c
+++ b/src/hw/nvme.c
@@ -318,7 +318,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);
+    u32 length = 1 + (ctrl->reg->cap & 0xffff);
     if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
         length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);

@@ -362,7 +362,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);
+    u32 length = 1 + (ctrl->reg->cap & 0xffff);
     if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
         length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);

-- 
2.17.1


On Tue, Aug 21, 2018 at 6:52 AM Matt DeVillier <matt.devillier at gmail.com>
wrote:

> On Mon, Aug 20, 2018 at 11:16 PM Kevin O'Connor <kevin at koconnor.net>
> 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 <matt.devillier at gmail.com>
>> > ---
>> >  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);
>> > --
>> > 2.17.1
>>
>> -Kevin
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/seabios/attachments/20180821/3b485f24/attachment-0001.html>