All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-rc v2] IB/qib: Protect from buffer overflow in struct qib_user_sdma_pkt fields
@ 2021-10-12 15:23 Dennis Dalessandro
  2021-10-12 15:40 ` Jason Gunthorpe
  0 siblings, 1 reply; 2+ messages in thread
From: Dennis Dalessandro @ 2021-10-12 15:23 UTC (permalink / raw)
  To: jgg, dledford; +Cc: linux-rdma, Mike Marciniszyn, Ilja Van Sprundel

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

Overflowing either addrlimit 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.

Fixes: f931551bafe1 ("IB/qib: Add new qib driver for QLogic PCIe InfiniBand adapters")
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>

---
Changes from v0:

Incorporate Jason's suggestions and update commit message. Also added on the
fixes line. Mike identified a different commit that is more directly
responsible.
---
 drivers/infiniband/hw/qib/qib_user_sdma.c |   38 +++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c
index a67599b..6af9764 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;
 
@@ -885,7 +885,11 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
 
 			npages += qib_user_sdma_num_pages(&iov[idx]);
 
-			bytes_togo += slen;
+			if (check_add_overflow(bytes_togo, slen, &bytes_togo) ||
+			    bytes_togo > type_max(typeof(pkt->bytes_togo))) {
+				ret = -EINVAL;
+				goto free_pbc;
+			}
 			pktnwc += slen >> 2;
 			idx++;
 			nfrags++;
@@ -904,11 +908,15 @@ 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, sz, addrlimit;
 
 			n = npages*((2*PAGE_SIZE/frag_size)+1);
+
 			pktsize = struct_size(pkt, addr, n);
+			if (pktsize == SIZE_MAX) {
+				ret = -EINVAL;
+				goto free_pbc;
+			}
 
 			/*
 			 * Determine if this is tid-sdma or just sdma.
@@ -923,14 +931,24 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
 			else
 				tidsmsize = 0;
 
-			pkt = kmalloc(pktsize+tidsmsize, GFP_KERNEL);
+			if (check_add_overflow(pktsize, tidsmsize, &sz)) {
+				ret = -EINVAL;
+				goto free_pbc;
+			}
+			pkt = kmalloc(sz, GFP_KERNEL);
 			if (!pkt) {
 				ret = -ENOMEM;
 				goto free_pbc;
 			}
 			pkt->largepkt = 1;
 			pkt->frag_size = frag_size;
-			pkt->addrlimit = n + 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;
+			}
+			pkt->addrlimit = addrlimit;
 
 			if (tiddma) {
 				char *tidsm = (char *)pkt + pktsize;


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH for-rc v2] IB/qib: Protect from buffer overflow in struct qib_user_sdma_pkt fields
  2021-10-12 15:23 [PATCH for-rc v2] IB/qib: Protect from buffer overflow in struct qib_user_sdma_pkt fields Dennis Dalessandro
@ 2021-10-12 15:40 ` Jason Gunthorpe
  0 siblings, 0 replies; 2+ messages in thread
From: Jason Gunthorpe @ 2021-10-12 15:40 UTC (permalink / raw)
  To: Dennis Dalessandro
  Cc: dledford, linux-rdma, Mike Marciniszyn, Ilja Van Sprundel

On Tue, Oct 12, 2021 at 11:23:31AM -0400, Dennis Dalessandro wrote:
> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> 
> Overflowing either addrlimit 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.
> 
> Fixes: f931551bafe1 ("IB/qib: Add new qib driver for QLogic PCIe InfiniBand adapters")
> 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>
> 
> Changes from v0:
> 
> Incorporate Jason's suggestions and update commit message. Also added on the
> fixes line. Mike identified a different commit that is more directly
> responsible.
>  drivers/infiniband/hw/qib/qib_user_sdma.c |   38 +++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c
> index a67599b..6af9764 100644
> +++ 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;
>  
> @@ -885,7 +885,11 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
>  
>  			npages += qib_user_sdma_num_pages(&iov[idx]);
>  
> -			bytes_togo += slen;
> +			if (check_add_overflow(bytes_togo, slen, &bytes_togo) ||
> +			    bytes_togo > type_max(typeof(pkt->bytes_togo))) {
> +				ret = -EINVAL;
> +				goto free_pbc;
> +			}
>  			pktnwc += slen >> 2;
>  			idx++;
>  			nfrags++;
> @@ -904,11 +908,15 @@ 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, sz, addrlimit;
>  
>  			n = npages*((2*PAGE_SIZE/frag_size)+1);
> +
>  			pktsize = struct_size(pkt, addr, n);
> +			if (pktsize == SIZE_MAX) {
> +				ret = -EINVAL;
> +				goto free_pbc;
> +			}

since pktsize directly flows into another check_add_overflow which
flows into a kmalloc this hunk isn't needed. kmalloc always fails for
SIZE_MAX

Jason

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-10-12 15:41 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12 15:23 [PATCH for-rc v2] IB/qib: Protect from buffer overflow in struct qib_user_sdma_pkt fields Dennis Dalessandro
2021-10-12 15:40 ` Jason Gunthorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.