| Message ID | 20240306095430.1782163-1-Ilia.Gavrilov@infotecs.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function | expand |
On Wed, 2024-03-06 at 09:57 +0000, Gavrilov Ilia wrote: > The 'len' variable can't be negative because all 'min_t' parameters > cast to unsigned int, and then the minimum one is chosen. The above is incorrect, as the 'len' variable is a signed integer The same applies to the following patches. Cheers, Paolo
On 3/6/24 14:36, Paolo Abeni wrote:
> The above is incorrect, as the 'len' variable is a signed integer
I mean, if 'len' is negative then after this expression
len = min_t(unsigned int, len, sizeof(int));
the 'len' variable will be equal to sizeof(int) == 4
and the statement
if (len < 0) return -EINVAL;
might be unreachable during program execution.
Hello Paolo, On Wed, Mar 6, 2024 at 7:36 PM Paolo Abeni <pabeni@redhat.com> wrote: > > On Wed, 2024-03-06 at 09:57 +0000, Gavrilov Ilia wrote: > > The 'len' variable can't be negative because all 'min_t' parameters > > cast to unsigned int, and then the minimum one is chosen. > > The above is incorrect, as the 'len' variable is a signed integer The 'len' variable should be converted to the non-negative value as this sentence: len = min_t(unsigned int, len, sizeof(int)); See the comments of min_t(): return minimum of two values, using the specified type. After executing the above code, it doesn't make sense to test if 'len < 0', I think. Thanks, Jason > > > The same applies to the following patches. > > Cheers, > > Paolo > >
On Wed, Mar 6, 2024 at 12:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > Hello Paolo, > > On Wed, Mar 6, 2024 at 7:36 PM Paolo Abeni <pabeni@redhat.com> wrote: > > > > On Wed, 2024-03-06 at 09:57 +0000, Gavrilov Ilia wrote: > > > The 'len' variable can't be negative because all 'min_t' parameters > > > cast to unsigned int, and then the minimum one is chosen. > > > > The above is incorrect, as the 'len' variable is a signed integer > > The 'len' variable should be converted to the non-negative value as > this sentence: > > len = min_t(unsigned int, len, sizeof(int)); > > See the comments of min_t(): return minimum of two values, using the > specified type. > > After executing the above code, it doesn't make sense to test if 'len > < 0', I think. This is essentially dead (defensive ?) code. Most compilers optimize this completely, no big deal.
On Wed, Mar 06, 2024 at 11:54:40AM +0000, Gavrilov Ilia wrote: > On 3/6/24 14:36, Paolo Abeni wrote: > > The above is incorrect, as the 'len' variable is a signed integer > > I mean, if 'len' is negative then after this expression > len = min_t(unsigned int, len, sizeof(int)); > the 'len' variable will be equal to sizeof(int) == 4 > and the statement > if (len < 0) return -EINVAL; > might be unreachable during program execution. Hi Gavrilov and Paolo, I could be missing something obvious but it seems to me that this is correct. Although perhaps we could try rewording the patch description to make things a bit clearer. Here is my attempt at that: The 'len' variable can't be negative when assigned the result of 'min_t' because all 'min_t' parameters are cast to unsigned int, and then the minimum one is chosen. To fix the logic, check 'len' as read from 'optlen', where the types of relevant variables are (signed) int. FWIIW, I see four similar patches on netdev this morning. It does seem to me that they are all valid fixes. But if they need to be reposted, or there are more coming, then I think it would be useful to bundle them up, say into batches of 10, and send as patch-sets. This may help with fragmentation of review of what seems to be the same change in multiple places.
С уважением, Илья Гаврилов Ведущий программист Отдел разработки АО "ИнфоТеКС" в г. Санкт-Петербург 127287, г. Москва, Старый Петровско-Разумовский проезд, дом 1/23, стр. 1 T: +7 495 737-61-92 ( доб. 4921) Ф: +7 495 737-72-78 Ilia.Gavrilov@infotecs.ru www.infotecs.ru On 3/7/24 11:40, Simon Horman wrote: > On Wed, Mar 06, 2024 at 11:54:40AM +0000, Gavrilov Ilia wrote: >> On 3/6/24 14:36, Paolo Abeni wrote: >>> The above is incorrect, as the 'len' variable is a signed integer >> >> I mean, if 'len' is negative then after this expression >> len = min_t(unsigned int, len, sizeof(int)); >> the 'len' variable will be equal to sizeof(int) == 4 >> and the statement >> if (len < 0) return -EINVAL; >> might be unreachable during program execution. > > Hi Gavrilov and Paolo, > > I could be missing something obvious but it seems to me that this is correct. > Although perhaps we could try rewording the patch description to > make things a bit clearer. Here is my attempt at that: > > The 'len' variable can't be negative when assigned the result of > 'min_t' because all 'min_t' parameters are cast to unsigned int, > and then the minimum one is chosen. > > To fix the logic, check 'len' as read from 'optlen', > where the types of relevant variables are (signed) int. > > FWIIW, I see four similar patches on netdev this morning. > It does seem to me that they are all valid fixes. > But if they need to be reposted, or there are more coming, > then I think it would be useful to bundle them up, > say into batches of 10, and send as patch-sets. > > This may help with fragmentation of review of what seems > to be the same change in multiple places. > > Hi Simon, thank you for your answer. I'll reword the patch description and repost a series of patches in V2. I also found a couple of places with the same problem.
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index c82dc42f57c6..a4f418592314 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -4010,11 +4010,11 @@ int do_tcp_getsockopt(struct sock *sk, int level, if (copy_from_sockptr(&len, optlen, sizeof(int))) return -EFAULT; - len = min_t(unsigned int, len, sizeof(int)); - if (len < 0) return -EINVAL; + len = min_t(unsigned int, len, sizeof(int)); + switch (optname) { case TCP_MAXSEG: val = tp->mss_cache;
The 'len' variable can't be negative because all 'min_t' parameters cast to unsigned int, and then the minimum one is chosen. To fix it, move the if statement higher. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru> --- net/ipv4/tcp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)