On Mon, Oct 04, 2021 at 07:56:25AM -0400, Dennis Dalessandro wrote:
> From: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxxxxxxxxxxxxx>
>
> Add protection for bytes_togo and n to avoid going beyond variables
> in PSM pkt structure.
>
> 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>
> 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
