[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c9dd0486-aacc-4263-bcce-630fad445e72@infotecs.ru>
Date: Wed, 6 Mar 2024 13:46:07 +0000
From: Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru>
To: Tom Parkin <tparkin@...alix.com>
CC: James Chapman <jchapman@...alix.com>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "lvc-project@...uxtesting.org"
<lvc-project@...uxtesting.org>
Subject: Re: [PATCH net-next] l2tp: fix incorrect parameter validation in the
pppol2tp_getsockopt() function
On 3/6/24 16:14, Tom Parkin wrote:
> On Wed, Mar 06, 2024 at 09:58:10 +0000, Gavrilov Ilia wrote:
>> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
>> index f011af6601c9..6146e4e67bbb 100644
>> --- a/net/l2tp/l2tp_ppp.c
>> +++ b/net/l2tp/l2tp_ppp.c
>> @@ -1356,11 +1356,11 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname,
>> if (get_user(len, optlen))
>> return -EFAULT;
>>
>> - len = min_t(unsigned int, len, sizeof(int));
>> -
>> if (len < 0)
>> return -EINVAL;
>>
>> + len = min_t(unsigned int, len, sizeof(int));
>> +
>> err = -ENOTCONN;
>> if (!sk->sk_user_data)
>> goto end;
>
> I think this code in l2tp_ppp.c has probably been inspired by a
> similar implementations elsewhere in the kernel -- for example
> net/ipv4/udp.c udp_lib_getsockopt does the same thing, and apparently
> has been that way since the dawn of git time.
>
> I note however that plenty of other getsockopt implementations which
> are using min_t(unsigned int, len, sizeof(int)) don't check the length
> value at all: as an example, net/ipv6/raw.c do_rawv6_getsockopt.
>
> As it stands right now in the l2tp_ppp.c code, I think the check on
> len will end up doing nothing, as you point out.
>
> So moving the len check to before the min_t() call may in theory
> possibly catch out (insane?) userspace code passing in negative
> numbers which may "work" with the current kernel code.
>
> I wonder whether its safer therefore to remove the len check
> altogether?
Thank you for answer.
In my opinion, it is better to leave the 'len' check. This way it will
be easier for the user to understand where the error is.
Powered by blists - more mailing lists