[PATCH for-rc] IB/qib: Fix issues noted by fuzzing on the iovecs

[PATCH for-rc] IB/qib: Fix issues noted by fuzzing on the iovecs

[Dennis Dalessandro]

From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>

Add protection for bytes_togo and n to avoid going beyond variables
in PSM pkt structure.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
---
 drivers/infiniband/hw/qib/qib_user_sdma.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c
index a67599b..144c3de 100644
--- a/drivers/infiniband/hw/qib/qib_user_sdma.c
+++ b/drivers/infiniband/hw/qib/qib_user_sdma.c
@@ -602,7 +602,7 @@ static int qib_user_sdma_coalesce(const struct qib_devdata *dd,
 /*
  * How many pages in this iovec element?
  */
-static int qib_user_sdma_num_pages(const struct iovec *iov)
+static size_t qib_user_sdma_num_pages(const struct iovec *iov)
 {
 	const unsigned long addr  = (unsigned long) iov->iov_base;
 	const unsigned long  len  = iov->iov_len;
@@ -658,7 +658,7 @@ static void qib_user_sdma_free_pkt_frag(struct device *dev,
 static int qib_user_sdma_pin_pages(const struct qib_devdata *dd,
 				   struct qib_user_sdma_queue *pq,
 				   struct qib_user_sdma_pkt *pkt,
-				   unsigned long addr, int tlen, int npages)
+				   unsigned long addr, int tlen, size_t npages)
 {
 	struct page *pages[8];
 	int i, j;
@@ -722,7 +722,7 @@ static int qib_user_sdma_pin_pkt(const struct qib_devdata *dd,
 	unsigned long idx;
 
 	for (idx = 0; idx < niov; idx++) {
-		const int npages = qib_user_sdma_num_pages(iov + idx);
+		const size_t npages = qib_user_sdma_num_pages(iov + idx);
 		const unsigned long addr = (unsigned long) iov[idx].iov_base;
 
 		ret = qib_user_sdma_pin_pages(dd, pq, pkt, addr,
@@ -824,8 +824,8 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
 		unsigned pktnw;
 		unsigned pktnwc;
 		int nfrags = 0;
-		int npages = 0;
-		int bytes_togo = 0;
+		size_t npages = 0;
+		size_t bytes_togo = 0;
 		int tiddma = 0;
 		int cfur;
 
@@ -878,7 +878,7 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
 			const unsigned long faddr =
 				(unsigned long) iov[idx].iov_base;
 
-			if (slen & 3 || faddr & 3 || !slen) {
+			if (slen & 3 || faddr & 3 || !slen || bytes_togo >= (U32_MAX-slen)) {
 				ret = -EINVAL;
 				goto free_pbc;
 			}
@@ -904,10 +904,14 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
 		}
 
 		if (frag_size) {
-			int tidsmsize, n;
-			size_t pktsize;
+			size_t tidsmsize, n, pktsize;
 
 			n = npages*((2*PAGE_SIZE/frag_size)+1);
+			if (n >= (U16_MAX - ARRAY_SIZE(pkt->addr))) {
+				ret = -EINVAL;
+				goto free_pbc;
+			}
+
 			pktsize = struct_size(pkt, addr, n);
 
 			/*

Re: [PATCH for-rc] IB/qib: Fix issues noted by fuzzing on the iovecs

[Dennis Dalessandro]

On 10/4/21 7:56 AM, Dennis Dalessandro wrote:
> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> 
> Add protection for bytes_togo and n to avoid going beyond variables
> in PSM pkt structure.
> 
> Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>

Might want to add:

Fixes: f931551bafe1 ("IB/qib: Add new qib driver for QLogic PCIe InfiniBand
adapters")

-Denny

Re: [PATCH for-rc] IB/qib: Fix issues noted by fuzzing on the iovecs

[Jason Gunthorpe]

On Mon, Oct 04, 2021 at 07:56:25AM -0400, Dennis Dalessandro wrote:
> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> 
> Add protection for bytes_togo and n to avoid going beyond variables
> in PSM pkt structure.
> 
> Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
>  drivers/infiniband/hw/qib/qib_user_sdma.c |   20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)

This commit message isn't good enough for a rc patch

    IB/qib: Protect from buffer overflow in struct qib_user_sdma_pkt::addr

    Overflowing either n or bytes_togo can allow userspace to trigger a buffer
    overflow of kernel memory. Check for overflows in all the places
    doing math on user controlled buffers

Don't forget the Fixes line.

  
> @@ -878,7 +878,7 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
>  			const unsigned long faddr =
>  				(unsigned long) iov[idx].iov_base;
>  
> -			if (slen & 3 || faddr & 3 || !slen) {
> +			if (slen & 3 || faddr & 3 || !slen || bytes_togo >= (U32_MAX-slen)) {
>  				ret = -EINVAL;
>  				goto free_pbc;
>  			}

This is not the expected way to write overflow checks these days, like this:

-                       bytes_togo += slen;
+                       if (check_add_overflow(bytes_togo, slen, &bytes_togo)) {
+                               ret = -EINVAL;
+                               goto free_pbc;
+                       }


> @@ -904,10 +904,14 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
>  		}
>  
>  		if (frag_size) {
> -			int tidsmsize, n;
> -			size_t pktsize;
> +			size_t tidsmsize, n, pktsize;
>  
>  			n = npages*((2*PAGE_SIZE/frag_size)+1);
> +			if (n >= (U16_MAX - ARRAY_SIZE(pkt->addr))) {
> +				ret = -EINVAL;
> +				goto free_pbc;
> +			}
> +
>  			pktsize = struct_size(pkt, addr, n);

And I think this is supposed to be more like this:

-                       if (n >= (U16_MAX - ARRAY_SIZE(pkt->addr))) {
+                       if (check_add_overflow(n, ARRAY_SIZE(pkt->addr),
+                                              &addrlimit) ||
+                           addrlimit > type_max(typeof(pkt->addrlimit))) {
                                ret = -EINVAL;
-                               goto free_pbc;
+                               goto free pbc;
                        }

And this is probably missing too:

-                       pkt = kmalloc(pktsize+tidsmsize, GFP_KERNEL);
+                       pktsize = struct_size(pkt, addr, n);
+                       if (check_add_overflow(pktsize, tidsmsize,
+                                              &allocsize)) {
+                               ret = -EINVAL;
+                               goto free_pbc;
+                       }
+                       pkt = kmalloc(allocsize, GFP_KERNEL);

The struct_size() of this:

struct qib_user_sdma_pkt {
	struct {
	} addr[4];   /* max pages, any more and we coalesce */
};

Is also super weird, that addr[4] should typically be [0]

Jason