| Message ID | 20211004115625.118981.81200.stgit@awfm-01.cornelisnetworks.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [for-rc] IB/qib: Fix issues noted by fuzzing on the iovecs | expand |
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
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
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); /*