[PATCH net-next v1 0/1] ppp: Fix KMSAN uninit-value warning

[PATCH net-next v1 0/1] ppp: Fix KMSAN uninit-value warning

[Jiayuan Chen]

Syzbot caught an "KMSAN: uninit-value" warning [1], which is caused by the
ppp driver not initializing a 2-byte header when using socket filters.

Here's a detailed explanation:

1. PPP protocol format
The PPP protocol format looks like this:

|<--------------------------      7 - 1508 bytes      --------------------------->|
+---0x7E---+---0xFF---+---0x03---+----------+---------------+----------+---0x7E----
|   Flag   | Address  | Control  | Protocol | Information   |   FCS    |   Flag   |
| 01111110 | 11111111 | 00000011 | 8/16bits |      *        | 16 bits  | 01111110 |
+----------+----------+----------+----------+---------------+----------+-----------


2. Normal BPF program
For example, when filtering IP over PPP, libpcap generates BPF
instructions like this:

(000) ldh [2]
(001) jeq #0x21 jt 2 jf 3
(002) ret #65535
(003) ret #0

2 bytes data are skipped by bpf program and then bpf program reads the
'Protocol' field to determine if it's an IP packet. Clearly, libpcap
assumes the packet starts from the Address field, just like the comment in
'drivers/net/ppp/ppp_generic.c':
/* the filter instructions are constructed assuming
   a four-byte PPP header on each packet */

Corresponding libpcap code is here:
https://github.com/the-tcpdump-group/libpcap/blob/master/gencode.c#L1421


3. Current problem
The problem is that the skb->data generated by ppp_write() starts from the
'Protocol' field.

To correctly use the BPF filter program, a 2-byte header is added to
simulate the presence of Address and Control fields. And then, after
running the socket filter, it's restored:

1768 *(u8 *)skb_push(skb, 2) = 1;
1770 bpf_prog_run()
1782 skb_pull(skb, 2);

The thing is, only one byte of the new 2-byte header is initialized. For
normal BPF programs generated by libpcap, uninitialized data won't be
used, so it's not a problem.

However, for carefully crafted BPF programs, such as those generated by
syzkaller [2], which start reading from offset 0, the uninitialized data
will be used and caught by KMSAN.

4. Fix
The fix is simple: initialize the entire 2-byte header.

[1] https://syzkaller.appspot.com/bug?extid=853242d9c9917165d791
[2] https://syzkaller.appspot.com/text?tag=ReproC&x=11994913980000

Jiayuan Chen (1):
  ppp: Fix KMSAN warning by initializing 2-byte header

 drivers/net/ppp/ppp_generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.47.1

[PATCH net-next v1 1/1] ppp: Fix KMSAN warning by initializing 2-byte header

[Jiayuan Chen]

The ppp program adds a 2-byte pseudo-header for socket filters, which is
normally skipped by regular BPF programs, causing no issues.

However, for abnormal BPF programs that use these uninitialized 2 bytes,
a KMSAN warning is triggered.

Reported-by: syzbot+853242d9c9917165d791@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/bpf/000000000000dea025060d6bc3bc@google.com/
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 drivers/net/ppp/ppp_generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4583e15ad03a..a913403d5847 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1765,7 +1765,7 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
 		/* check if we should pass this packet */
 		/* the filter instructions are constructed assuming
 		   a four-byte PPP header on each packet */
-		*(u8 *)skb_push(skb, 2) = 1;
+		*(u16 *)skb_push(skb, 2) = 1;
 		if (ppp->pass_filter &&
 		    bpf_prog_run(ppp->pass_filter, skb) == 0) {
 			if (ppp->debug & 1)
@@ -2489,7 +2489,7 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
 			if (skb_unclone(skb, GFP_ATOMIC))
 				goto err;
 
-			*(u8 *)skb_push(skb, 2) = 0;
+			*(u16 *)skb_push(skb, 2) = 0;
 			if (ppp->pass_filter &&
 			    bpf_prog_run(ppp->pass_filter, skb) == 0) {
 				if (ppp->debug & 1)
-- 
2.47.1

Re: [PATCH net-next v1 1/1] ppp: Fix KMSAN warning by initializing 2-byte header

[Jakub Kicinski]

On Tue, 18 Feb 2025 21:31:44 +0800 Jiayuan Chen wrote:
> -		*(u8 *)skb_push(skb, 2) = 1;
> +		*(u16 *)skb_push(skb, 2) = 1;

This will write the 1 to a different byte now, on big endian machines.
Probably doesn't matter but I doubt it's intentional?
-- 
pw-bot: cr

Re: [PATCH net-next v1 1/1] ppp: Fix KMSAN warning by initializing 2-byte header

[Jiayuan Chen]

On Thu, Feb 20, 2025 at 03:27:03PM -0800, Jakub Kicinski wrote:
> On Tue, 18 Feb 2025 21:31:44 +0800 Jiayuan Chen wrote:
> > -		*(u8 *)skb_push(skb, 2) = 1;
> > +		*(u16 *)skb_push(skb, 2) = 1;
> 
> This will write the 1 to a different byte now, on big endian machines.
> Probably doesn't matter but I doubt it's intentional?
> -- 
> pw-bot: cr
You are correct that I assigned the value in a way that produces different
data on big-endian and little-endian systems, although it doesn't cause
any issues.
I think it's better to assign it correctly according to the corresponding
header and add more comments to avoid confusion for other developers in
the future.

Re: [PATCH net-next v1 1/1] ppp: Fix KMSAN warning by initializing 2-byte header

[Simon Horman]

On Fri, Feb 21, 2025 at 09:48:33AM +0800, Jiayuan Chen wrote:
> On Thu, Feb 20, 2025 at 03:27:03PM -0800, Jakub Kicinski wrote:
> > On Tue, 18 Feb 2025 21:31:44 +0800 Jiayuan Chen wrote:
> > > -		*(u8 *)skb_push(skb, 2) = 1;
> > > +		*(u16 *)skb_push(skb, 2) = 1;
> > 
> > This will write the 1 to a different byte now, on big endian machines.
> > Probably doesn't matter but I doubt it's intentional?
> > -- 
> > pw-bot: cr
> You are correct that I assigned the value in a way that produces different
> data on big-endian and little-endian systems, although it doesn't cause
> any issues.
> I think it's better to assign it correctly according to the corresponding
> header and add more comments to avoid confusion for other developers in
> the future.

I agree correctness is good.

Perhaps I am over-thinking things, but does the following approach
achieve both of the following?

a) Initialise both bytes.
b) Place the 1 consistently on both big and little endian hosts,
   as is the case without this patch (which I assume is correct).

	*(__be16 *)skb_push(skb, 2) = cpu_to_be16(1);