<div dir="ltr"><div>From 2741d96d07855c3bc904d6ce47c82391039bca87 Mon Sep 17 00:00:00 2001</div><div>From: Matt DeVillier <<a href="mailto:matt.devillier@puri.sm">matt.devillier@puri.sm</a>></div><div>Date: Tues, 21 Aug 2018 10:00:53 -0500</div><div>Subject: [PATCH v2 1/1] nvme: fix I/O queue length calculation overflow</div><div><br></div><div>Commit cd47172 changed the I/O queue length calculation to use the</div><div>Maximum Queue Entries Supported (MQES) value from the capabilities</div><div>register, plus one, with a maximum value of NVME_PAGE_SIZE.</div><div><br></div><div>An unintended effect from this is that due to length being an unsigned</div><div>16-bit int, a MQES value of 0xFFFF yields a length of zero, resulting</div><div>in the queue allocation failing. Fix this by changing length to a u32.</div><div><br></div><div>TEST: build/boot on a Purism Librem13v2 with a MyDigitalSSD BPX NVMe</div><div>drive, which reports a MQES of 0xFFFF. Verify NVMe drive present in</div><div>boot menu and OS boots successfully.</div><div><br></div><div>Signed-off-by: Matt DeVillier <<a href="mailto:matt.devillier@puri.sm">matt.devillier@puri.sm</a>></div><div>---</div><div> src/hw/nvme.c | 4 ++--</div><div> 1 file changed, 2 insertions(+), 2 deletions(-)</div><div><br></div><div>diff --git a/src/hw/nvme.c b/src/hw/nvme.c</div><div>index e6d739d..2e3aa38 100644</div><div>--- a/src/hw/nvme.c</div><div>+++ b/src/hw/nvme.c</div><div>@@ -318,7 +318,7 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct nvme_cq *cq, u16 q_idx)</div><div> {</div><div> int rc;</div><div> struct nvme_sqe *cmd_create_cq;</div><div>- u16 length = 1 + (ctrl->reg->cap & 0xffff);</div><div>+ u32 length = 1 + (ctrl->reg->cap & 0xffff);</div><div> if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))</div><div> length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);</div><div> </div><div>@@ -362,7 +362,7 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct nvme_sq *sq, u16 q_idx, struct</div><div> {</div><div> int rc;</div><div> struct nvme_sqe *cmd_create_sq;</div><div>- u16 length = 1 + (ctrl->reg->cap & 0xffff);</div><div>+ u32 length = 1 + (ctrl->reg->cap & 0xffff);</div><div> if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))</div><div> length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);</div><div> </div><div>-- </div><div>2.17.1</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Aug 21, 2018 at 6:52 AM Matt DeVillier <<a href="mailto:matt.devillier@gmail.com">matt.devillier@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 20, 2018 at 11:16 PM Kevin O'Connor <<a href="mailto:kevin@koconnor.net" target="_blank">kevin@koconnor.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sun, Aug 12, 2018 at 03:42:40PM -0500, Matt DeVillier wrote:<br>
> Commit cd47172 changed the I/O queue length calculation to use the<br>
> Maximum Queue Entries Supported (MQES) value from the capabilities<br>
> register, plus one, with a maximum value of NVME_PAGE_SIZE.<br>
> <br>
> An unintended effect from this is that a MQES value of 0xFFFF yields<br>
> a length of zero, resulting in the queue allocation failing. Fix this<br>
> by checking for a zero length when checking that the length exceeds<br>
> NVME_PAGE_SIZE, and setting to NVME_PAGE_SIZE.<br>
> <br>
> TEST: build/boot on a Purism Librem13v2 with a MyDigitalSSD BPX NVMe<br>
> drive, which reports a MQES of 0xFFFF. Verify NVMe drive present in<br>
> boot menu and OS boots successfully.<br>
> <br>
> Signed-off-by: Matt DeVillier <<a href="mailto:matt.devillier@gmail.com" target="_blank">matt.devillier@gmail.com</a>><br>
> ---<br>
> src/hw/nvme.c | 4 ++--<br>
> 1 file changed, 2 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/hw/nvme.c b/src/hw/nvme.c<br>
> index e6d739d..c2d6863 100644<br>
> --- a/src/hw/nvme.c<br>
> +++ b/src/hw/nvme.c<br>
> @@ -319,7 +319,7 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct<br>
> nvme_cq *cq, u16 q_idx)<br>
> int rc;<br>
> struct nvme_sqe *cmd_create_cq;<br>
> u16 length = 1 + (ctrl->reg->cap & 0xffff);<br>
> - if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))<br>
> + if (length == 0 || length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))<br>
> length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);<br>
<br>
Thanks. Instead of changing the if statement, could we just change<br>
length here to a u32 to avoid this issue?<br></blockquote><div><br></div><div>I can't think of why not, I'll test this method and submit an updated patchset if so - thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> <br>
> rc = nvme_init_cq(ctrl, cq, q_idx, length);<br>
> @@ -363,7 +363,7 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct<br>
> nvme_sq *sq, u16 q_idx, struct<br>
> int rc;<br>
> struct nvme_sqe *cmd_create_sq;<br>
> u16 length = 1 + (ctrl->reg->cap & 0xffff);<br>
> - if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))<br>
> + if (length == 0 || length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))<br>
> length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);<br>
> <br>
> rc = nvme_init_sq(ctrl, sq, q_idx, length, cq);<br>
> -- <br>
> 2.17.1<br>
<br>
-Kevin<br>
</blockquote></div></div>
</blockquote></div>