On Tue, Oct 12, 2021 at 11:23:31AM -0400, Dennis Dalessandro wrote:
> From: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx>
>
> 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
