Re: [bug report] net: pktgen: fix access outside of user given buffer in pktgen_if_write()

[Peter Seiderer <ps.report@gmx.net>]

Hello Dan,

On Thu, 6 Mar 2025 12:48:51 +0300, Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Hello Peter Seiderer,
>
> Commit c5cdbf23b84c ("net: pktgen: fix access outside of user given
> buffer in pktgen_if_write()") from Feb 27, 2025 (linux-next), leads
> to the following Smatch static checker warning:
>
> 	net/core/pktgen.c:877 get_imix_entries()
> 	warn: check that incremented offset 'i' is capped
>
> net/core/pktgen.c
>     842 static ssize_t get_imix_entries(const char __user *buffer,
>     843                                 size_t maxlen,
>     844                                 struct pktgen_dev *pkt_dev)
>     845 {
>     846         size_t i = 0, max;
>     847         ssize_t len;
>     848         char c;
>     849
>     850         pkt_dev->n_imix_entries = 0;
>     851
>     852         do {
>     853                 unsigned long weight;
>     854                 unsigned long size;
>     855
>     856                 if (pkt_dev->n_imix_entries >= MAX_IMIX_ENTRIES)
>     857                         return -E2BIG;
>     858
>     859                 max = min(10, maxlen - i);
>     860                 len = num_arg(&buffer[i], max, &size);
>     861                 if (len < 0)
>     862                         return len;
>     863                 i += len;
>     864                 if (i >= maxlen)
>
> Smatch wants this check to be done
>
>     865                         return -EINVAL;
>     866                 if (get_user(c, &buffer[i]))
>     867                         return -EFAULT;
>     868                 /* Check for comma between size_i and weight_i */
>     869                 if (c != ',')
>     870                         return -EINVAL;
>     871                 i++;
>
> again after this i++.
>
>     872
>     873                 if (size < 14 + 20 + 8)
>     874                         size = 14 + 20 + 8;
>     875
>     876                 max = min(10, maxlen - i);
> --> 877                 len = num_arg(&buffer[i], max, &weight);
>     878                 if (len < 0)
>     879                         return len;
>     880                 if (weight <= 0)
>     881                         return -EINVAL;

Smatch ist right on this one, num_arg is called with an invalid buffer position,
but at the same time Smatch is wrong as even in case of i == maxlen, max
is calculated to zero, and inside num_arg the invalid buffer position is never
accessed (for loop capped by max == 0) and a zero value for weight is returned
leading to 'return -EINVAL'...

But yes, checking i against maxlen after the get_user/i++ step (here and at some
other locations) is more straight forward and easier to review (and easier to
check for correctness), will provide a patch fixing this the next days...

Thanks for providing the Smatch warning and explanation!

Regards,
Peter

>     882
>     883                 pkt_dev->imix_entries[pkt_dev->n_imix_entries].size = size;
>     884                 pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight;
>     885
>     886                 i += len;
>     887                 pkt_dev->n_imix_entries++;
>     888
>     889                 if (i >= maxlen)
>     890                         break;
>     891                 if (get_user(c, &buffer[i]))
>     892                         return -EFAULT;
>     893                 i++;
>     894         } while (c == ' ');
>     895
>     896         return i;
>     897 }
>
> regards,
> dan carpenter