[PATCH] nvme-pci: Fix mempool alloc size

[PATCH] nvme-pci: Fix mempool alloc size

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Convert the max size to bytes to match the units of the divisor that
calculates the worst-case number of PRP entries.

The result is used to determine how many PRP Lists are required. The
code was previously rounding this to 1 list, but we can require 2 in the
worst case. In that scenario, the driver would corrupt memory beyond the
size provided by the mempool.

While unlikely to occur (you'd need a 4MB in exactly 127 phys segments
on a queue that doesn't support SGLs), this memory corruption has been
observed by kfence.

Cc: Jens Axboe <axboe@kernel.dk>
Fixes: 943e942e6266f ("nvme-pci: limit max IO size and segments to avoid high order allocations")
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f0f8027644bbf..fa182fcd4c3e8 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -380,8 +380,8 @@ static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
  */
 static int nvme_pci_npages_prp(void)
 {
-	unsigned nprps = DIV_ROUND_UP(NVME_MAX_KB_SZ + NVME_CTRL_PAGE_SIZE,
-				      NVME_CTRL_PAGE_SIZE);
+	unsigned max_bytes = (NVME_MAX_KB_SZ * 1024) + NVME_CTRL_PAGE_SIZE;
+	unsigned nprps = DIV_ROUND_UP(max_bytes, NVME_CTRL_PAGE_SIZE);
 	return DIV_ROUND_UP(8 * nprps, PAGE_SIZE - 8);
 }
 
-- 
2.30.2

Re: [PATCH] nvme-pci: Fix mempool alloc size

[Jens Axboe]

On 12/19/22 11:59 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Convert the max size to bytes to match the units of the divisor that
> calculates the worst-case number of PRP entries.
> 
> The result is used to determine how many PRP Lists are required. The
> code was previously rounding this to 1 list, but we can require 2 in the
> worst case. In that scenario, the driver would corrupt memory beyond the
> size provided by the mempool.
> 
> While unlikely to occur (you'd need a 4MB in exactly 127 phys segments
> on a queue that doesn't support SGLs), this memory corruption has been
> observed by kfence.

Good catch!

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH] nvme-pci: Fix mempool alloc size

[Chaitanya Kulkarni]

On 12/19/22 10:59, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Convert the max size to bytes to match the units of the divisor that
> calculates the worst-case number of PRP entries.
> 
> The result is used to determine how many PRP Lists are required. The
> code was previously rounding this to 1 list, but we can require 2 in the
> worst case. In that scenario, the driver would corrupt memory beyond the
> size provided by the mempool.
> 
> While unlikely to occur (you'd need a 4MB in exactly 127 phys segments
> on a queue that doesn't support SGLs), this memory corruption has been
> observed by kfence.
> 
> Cc: Jens Axboe <axboe@kernel.dk>
> Fixes: 943e942e6266f ("nvme-pci: limit max IO size and segments to avoid high order allocations")
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---

hmm, surprising to see that we never caught this until today...

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] nvme-pci: Fix mempool alloc size

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Mon, Dec 19, 2022 at 10:59:06AM -0800, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>Convert the max size to bytes to match the units of the divisor that
>calculates the worst-case number of PRP entries.
>
>The result is used to determine how many PRP Lists are required. The
>code was previously rounding this to 1 list, but we can require 2 in the
>worst case. In that scenario, the driver would corrupt memory beyond the
>size provided by the mempool.
>
>While unlikely to occur (you'd need a 4MB in exactly 127 phys segments
>on a queue that doesn't support SGLs), this memory corruption has been
>observed by kfence.
>
>Cc: Jens Axboe <axboe@kernel.dk>
>Fixes: 943e942e6266f ("nvme-pci: limit max IO size and segments to avoid high order allocations")
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/pci.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>index f0f8027644bbf..fa182fcd4c3e8 100644
>--- a/drivers/nvme/host/pci.c
>+++ b/drivers/nvme/host/pci.c
>@@ -380,8 +380,8 @@ static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
>  */
> static int nvme_pci_npages_prp(void)
> {
>-	unsigned nprps = DIV_ROUND_UP(NVME_MAX_KB_SZ + NVME_CTRL_PAGE_SIZE,
>-				      NVME_CTRL_PAGE_SIZE);

Similar calculation is present in apple.c too.
Regardless, this looks good.
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] nvme-pci: Fix mempool alloc size

[Christoph Hellwig]

Thanks,

applied to nvme-6.2.