* [bug report] net: pktgen: fix access outside of user given buffer in pktgen_if_write()
@ 2025-03-06 9:48 Dan Carpenter
2025-03-07 15:08 ` Peter Seiderer
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2025-03-06 9:48 UTC (permalink / raw)
To: Peter Seiderer; +Cc: netdev
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;
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
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [bug report] net: pktgen: fix access outside of user given buffer in pktgen_if_write()
2025-03-06 9:48 [bug report] net: pktgen: fix access outside of user given buffer in pktgen_if_write() Dan Carpenter
@ 2025-03-07 15:08 ` Peter Seiderer
0 siblings, 0 replies; 2+ messages in thread
From: Peter Seiderer @ 2025-03-07 15:08 UTC (permalink / raw)
To: Dan Carpenter; +Cc: netdev
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-03-07 15:08 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-06 9:48 [bug report] net: pktgen: fix access outside of user given buffer in pktgen_if_write() Dan Carpenter
2025-03-07 15:08 ` Peter Seiderer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).