[PATCH] ppp: Prevent out-of-bounds access in ppp_sync_txmunge

[PATCH] ppp: Prevent out-of-bounds access in ppp_sync_txmunge

[Purva Yeshi]

Fix an issue detected by syzbot with KMSAN:

BUG: KMSAN: uninit-value in ppp_sync_txmunge
drivers/net/ppp/ppp_synctty.c:516 [inline]
BUG: KMSAN: uninit-value in ppp_sync_send+0x21c/0xb00
drivers/net/ppp/ppp_synctty.c:568

Ensure sk_buff is valid and has at least 3 bytes before accessing its
data field in ppp_sync_txmunge(). Without this check, the function may
attempt to read uninitialized or invalid memory, leading to undefined
behavior.

To address this, add a validation check at the beginning of the function
to safely handle cases where skb is NULL or too small. If either condition
is met, free the skb and return NULL to prevent processing an invalid
packet.

Reported-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=29fc8991b0ecb186cf40
Tested-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
---
 drivers/net/ppp/ppp_synctty.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
index 644e99fc3..e537ea3d9 100644
--- a/drivers/net/ppp/ppp_synctty.c
+++ b/drivers/net/ppp/ppp_synctty.c
@@ -506,6 +506,12 @@ ppp_sync_txmunge(struct syncppp *ap, struct sk_buff *skb)
 	unsigned char *data;
 	int islcp;
 
+	/* Ensure skb is not NULL and has at least 3 bytes */
+	if (!skb || skb->len < 3) {
+		kfree_skb(skb);
+		return NULL;
+	}
+
 	data  = skb->data;
 	proto = get_unaligned_be16(data);
 
-- 
2.34.1

Re: [PATCH] ppp: Prevent out-of-bounds access in ppp_sync_txmunge

[Kuniyuki Iwashima]

From: Purva Yeshi <purvayeshi550@gmail.com>
Date: Sun, 16 Feb 2025 11:34:46 +0530
> Fix an issue detected by syzbot with KMSAN:
> 
> BUG: KMSAN: uninit-value in ppp_sync_txmunge
> drivers/net/ppp/ppp_synctty.c:516 [inline]
> BUG: KMSAN: uninit-value in ppp_sync_send+0x21c/0xb00
> drivers/net/ppp/ppp_synctty.c:568
> 
> Ensure sk_buff is valid and has at least 3 bytes before accessing its
> data field in ppp_sync_txmunge(). Without this check, the function may
> attempt to read uninitialized or invalid memory, leading to undefined
> behavior.
> 
> To address this, add a validation check at the beginning of the function
> to safely handle cases where skb is NULL or too small. If either condition
> is met, free the skb and return NULL to prevent processing an invalid
> packet.
> 
> Reported-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=29fc8991b0ecb186cf40
> Tested-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
>  drivers/net/ppp/ppp_synctty.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
> index 644e99fc3..e537ea3d9 100644
> --- a/drivers/net/ppp/ppp_synctty.c
> +++ b/drivers/net/ppp/ppp_synctty.c
> @@ -506,6 +506,12 @@ ppp_sync_txmunge(struct syncppp *ap, struct sk_buff *skb)
>  	unsigned char *data;
>  	int islcp;
>  
> +	/* Ensure skb is not NULL and has at least 3 bytes */
> +	if (!skb || skb->len < 3) {

When is skb NULL ?


> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +
>  	data  = skb->data;
>  	proto = get_unaligned_be16(data);
>  
> -- 
> 2.34.1

Re: [PATCH] ppp: Prevent out-of-bounds access in ppp_sync_txmunge

[Purva Yeshi]

On 18/02/25 02:46, Kuniyuki Iwashima wrote:
> From: Purva Yeshi <purvayeshi550@gmail.com>
> Date: Sun, 16 Feb 2025 11:34:46 +0530
>> Fix an issue detected by syzbot with KMSAN:
>>
>> BUG: KMSAN: uninit-value in ppp_sync_txmunge
>> drivers/net/ppp/ppp_synctty.c:516 [inline]
>> BUG: KMSAN: uninit-value in ppp_sync_send+0x21c/0xb00
>> drivers/net/ppp/ppp_synctty.c:568
>>
>> Ensure sk_buff is valid and has at least 3 bytes before accessing its
>> data field in ppp_sync_txmunge(). Without this check, the function may
>> attempt to read uninitialized or invalid memory, leading to undefined
>> behavior.
>>
>> To address this, add a validation check at the beginning of the function
>> to safely handle cases where skb is NULL or too small. If either condition
>> is met, free the skb and return NULL to prevent processing an invalid
>> packet.
>>
>> Reported-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=29fc8991b0ecb186cf40
>> Tested-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
>> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
>> ---
>>   drivers/net/ppp/ppp_synctty.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
>> index 644e99fc3..e537ea3d9 100644
>> --- a/drivers/net/ppp/ppp_synctty.c
>> +++ b/drivers/net/ppp/ppp_synctty.c
>> @@ -506,6 +506,12 @@ ppp_sync_txmunge(struct syncppp *ap, struct sk_buff *skb)
>>   	unsigned char *data;
>>   	int islcp;
>>   
>> +	/* Ensure skb is not NULL and has at least 3 bytes */
>> +	if (!skb || skb->len < 3) {
> 
> When is skb NULL ?

skb pointer can be NULL in cases where memory allocation for the socket 
buffer fails, or if an upstream function incorrectly passes a NULL 
reference due to improper error handling.

Additionally, skb->len being less than 3 can occur if the received 
packet is truncated or malformed, leading to out-of-bounds memory access 
when attempting to read data[2].

> 
> 
>> +		kfree_skb(skb);
>> +		return NULL;
>> +	}
>> +
>>   	data  = skb->data;
>>   	proto = get_unaligned_be16(data);
>>   
>> -- 
>> 2.34.1

Re: [PATCH] ppp: Prevent out-of-bounds access in ppp_sync_txmunge

[Kuniyuki Iwashima]

From: Purva Yeshi <purvayeshi550@gmail.com>
Date: Tue, 18 Feb 2025 11:58:17 +0530
> On 18/02/25 02:46, Kuniyuki Iwashima wrote:
> > From: Purva Yeshi <purvayeshi550@gmail.com>
> > Date: Sun, 16 Feb 2025 11:34:46 +0530
> >> Fix an issue detected by syzbot with KMSAN:
> >>
> >> BUG: KMSAN: uninit-value in ppp_sync_txmunge
> >> drivers/net/ppp/ppp_synctty.c:516 [inline]
> >> BUG: KMSAN: uninit-value in ppp_sync_send+0x21c/0xb00
> >> drivers/net/ppp/ppp_synctty.c:568
> >>
> >> Ensure sk_buff is valid and has at least 3 bytes before accessing its
> >> data field in ppp_sync_txmunge(). Without this check, the function may
> >> attempt to read uninitialized or invalid memory, leading to undefined
> >> behavior.
> >>
> >> To address this, add a validation check at the beginning of the function
> >> to safely handle cases where skb is NULL or too small. If either condition
> >> is met, free the skb and return NULL to prevent processing an invalid
> >> packet.
> >>
> >> Reported-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
> >> Closes: https://syzkaller.appspot.com/bug?extid=29fc8991b0ecb186cf40
> >> Tested-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
> >> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> >> ---
> >>   drivers/net/ppp/ppp_synctty.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
> >> index 644e99fc3..e537ea3d9 100644
> >> --- a/drivers/net/ppp/ppp_synctty.c
> >> +++ b/drivers/net/ppp/ppp_synctty.c
> >> @@ -506,6 +506,12 @@ ppp_sync_txmunge(struct syncppp *ap, struct sk_buff *skb)
> >>   	unsigned char *data;
> >>   	int islcp;
> >>   
> >> +	/* Ensure skb is not NULL and has at least 3 bytes */
> >> +	if (!skb || skb->len < 3) {
> > 
> > When is skb NULL ?
> 
> skb pointer can be NULL in cases where memory allocation for the socket 
> buffer fails, or if an upstream function incorrectly passes a NULL 
> reference due to improper error handling.

Which caller passes NULL skb ?

If it's really possible, you'll see null-ptr-deref at

  data = skb->data;

below instead of KMSAN's uninit splat.


> 
> Additionally, skb->len being less than 3 can occur if the received 
> packet is truncated or malformed, leading to out-of-bounds memory access 
> when attempting to read data[2].
> 
> > 
> > 
> >> +		kfree_skb(skb);
> >> +		return NULL;
> >> +	}
> >> +
> >>   	data  = skb->data;
> >>   	proto = get_unaligned_be16(data);
> >>   
> >> -- 
> >> 2.34.1

Re: [PATCH] ppp: Prevent out-of-bounds access in ppp_sync_txmunge

[Purva Yeshi]

On 19/02/25 00:20, Kuniyuki Iwashima wrote:
> From: Purva Yeshi <purvayeshi550@gmail.com>
> Date: Tue, 18 Feb 2025 11:58:17 +0530
>> On 18/02/25 02:46, Kuniyuki Iwashima wrote:
>>> From: Purva Yeshi <purvayeshi550@gmail.com>
>>> Date: Sun, 16 Feb 2025 11:34:46 +0530
>>>> Fix an issue detected by syzbot with KMSAN:
>>>>
>>>> BUG: KMSAN: uninit-value in ppp_sync_txmunge
>>>> drivers/net/ppp/ppp_synctty.c:516 [inline]
>>>> BUG: KMSAN: uninit-value in ppp_sync_send+0x21c/0xb00
>>>> drivers/net/ppp/ppp_synctty.c:568
>>>>
>>>> Ensure sk_buff is valid and has at least 3 bytes before accessing its
>>>> data field in ppp_sync_txmunge(). Without this check, the function may
>>>> attempt to read uninitialized or invalid memory, leading to undefined
>>>> behavior.
>>>>
>>>> To address this, add a validation check at the beginning of the function
>>>> to safely handle cases where skb is NULL or too small. If either condition
>>>> is met, free the skb and return NULL to prevent processing an invalid
>>>> packet.
>>>>
>>>> Reported-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?extid=29fc8991b0ecb186cf40
>>>> Tested-by: syzbot+29fc8991b0ecb186cf40@syzkaller.appspotmail.com
>>>> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
>>>> ---
>>>>    drivers/net/ppp/ppp_synctty.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
>>>> index 644e99fc3..e537ea3d9 100644
>>>> --- a/drivers/net/ppp/ppp_synctty.c
>>>> +++ b/drivers/net/ppp/ppp_synctty.c
>>>> @@ -506,6 +506,12 @@ ppp_sync_txmunge(struct syncppp *ap, struct sk_buff *skb)
>>>>    	unsigned char *data;
>>>>    	int islcp;
>>>>    
>>>> +	/* Ensure skb is not NULL and has at least 3 bytes */
>>>> +	if (!skb || skb->len < 3) {
>>>
>>> When is skb NULL ?
>>
>> skb pointer can be NULL in cases where memory allocation for the socket
>> buffer fails, or if an upstream function incorrectly passes a NULL
>> reference due to improper error handling.
> 
> Which caller passes NULL skb ?
> 
> If it's really possible, you'll see null-ptr-deref at
> 
>    data = skb->data;
> 
> below instead of KMSAN's uninit splat.

Understood. I’ll check where the skb pointer is receiving uninitialized 
data.

> 
> 
>>
>> Additionally, skb->len being less than 3 can occur if the received
>> packet is truncated or malformed, leading to out-of-bounds memory access
>> when attempting to read data[2].
>>
>>>
>>>
>>>> +		kfree_skb(skb);
>>>> +		return NULL;
>>>> +	}
>>>> +
>>>>    	data  = skb->data;
>>>>    	proto = get_unaligned_be16(data);
>>>>    
>>>> -- 
>>>> 2.34.1