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

[PATCH net-next] l2tp: fix incorrect parameter validation in the pppol2tp_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: 3557baabf280 ("[L2TP]: PPP over L2TP driver core")
Signed-off-by: Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>
---
 net/l2tp/l2tp_ppp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

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?
-- 
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Gavrilov Ilia]

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.

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

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On  Wed, Mar 06, 2024 at 13:46:07 +0000, Gavrilov Ilia wrote:
> On 3/6/24 16:14, Tom Parkin wrote:
> > 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.

Fair enough.

My concern was that in doing so we add a new behaviour which userspace
may notice and care about, but realistically I'm probably being
paranoid to imagine that any such userspace exists.

Thanks for your work on l2tp_ppp.c :-)

Reviewed-by: Tom Parkin <tparkin@katalix.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]