[PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[Gavrilov Ilia]

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(-)

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;
-- 
2.39.2

Re: [PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[Paolo Abeni]

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

Re: [PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[Gavrilov Ilia]

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.

Re: [PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[Simon Horman]

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.

Re: [PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[Gavrilov Ilia]

С уважением,
Илья Гаврилов
Ведущий программист
Отдел разработки
АО "ИнфоТеКС" в г. Санкт-Петербург
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.

Re: [PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[Jason Xing]

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
>
>

Re: [PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp_getsockopt() function

[Eric Dumazet]

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.