From 2741d96d07855c3bc904d6ce47c82391039bca87 Mon Sep 17 00:00:00 2001
From: Matt DeVillier matt.devillier@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@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@gmail.com
wrote:
> On Mon, Aug 20, 2018 at 11:16 PM Kevin O'Connor
kevin@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@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
>>
>
Matt DeVillier