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

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

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

The following code can generate a PPP filter BPF program:
'''
struct bpf_program fp;
pcap_t *handle;
handle = pcap_open_dead(DLT_PPP_PPPD, 65535);
pcap_compile(handle, &fp, "ip and outbound", 0, 0);
bpf_dump(&fp, 1);
'''
Its output is:
'''
(000) ldh [2]
(001) jeq #0x21 jt 2 jf 5
(002) ldb [0]
(003) jeq #0x1 jt 4 jf 5
(004) ret #65535
(005) ret #0
'''

wen can find similar code at the following link:
https://github.com/ppp-project/ppp/blob/master/pppd/options.c#L1680
The maintainer of this code repository is also the original maintainer
of the ppp driver.


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

But the BPF program skips 2 bytes of data and then reads the 'Protocol'
field to determine if it's an IP packet 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 */

In the current PPP driver implementation, to correctly use the BPF filter
program, a 2-byte header is added, 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 issue is that only the first byte indicating direction is initialized,
while the second byte is not 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.

Cc: Paul Mackerras <paulus@samba.org>

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

---
v3 -> v4:
Use macro instead.
Use __be16 to suppress compilation warnings.

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

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

-- 
2.47.1

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

[Jiayuan Chen]

The PPP driver adds an extra 2-byte header to enable socket filters to run
correctly. However, the driver only initializes the first byte, which
indicates the direction. For normal BPF programs, this is not a problem
since they only read the first byte.

Nevertheless, for carefully crafted BPF programs, if they read the second
byte, this will trigger a KMSAN warning for reading uninitialized data.

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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4583e15ad03a..b4433badf03c 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -72,6 +72,10 @@
 #define PPP_PROTO_LEN	2
 #define PPP_LCP_HDRLEN	4
 
+/* These are fields recognized by libpcap */
+#define PPP_FILTER_OUTBOUND_TAG 0x0100
+#define PPP_FILTER_INBOUND_TAG  0x0000
+
 /*
  * An instance of /dev/ppp can be associated with either a ppp
  * interface unit or a ppp channel.  In both cases, file->private_data
@@ -1762,10 +1766,15 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
 
 	if (proto < 0x8000) {
 #ifdef CONFIG_PPP_FILTER
-		/* 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;
+		/* Check if we should pass this packet.
+		 * The filter instructions are constructed assuming
+		 * a four-byte PPP header on each packet. The first byte
+		 * indicates the direction, and the second byte is meaningless,
+		 * but we still need to initialize it to prevent crafted BPF
+		 * programs from reading them which would cause reading of
+		 * uninitialized data.
+		 */
+		*(__be16 *)skb_push(skb, 2) = htons(PPP_FILTER_OUTBOUND_TAG);
 		if (ppp->pass_filter &&
 		    bpf_prog_run(ppp->pass_filter, skb) == 0) {
 			if (ppp->debug & 1)
-- 
2.47.1

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

[Jakub Kicinski]

On Wed, 26 Feb 2025 09:36:58 +0800 Jiayuan Chen wrote:
> The PPP driver adds an extra 2-byte header to enable socket filters to run
> correctly. However, the driver only initializes the first byte, which
> indicates the direction. For normal BPF programs, this is not a problem
> since they only read the first byte.
> 
> Nevertheless, for carefully crafted BPF programs, if they read the second
> byte, this will trigger a KMSAN warning for reading uninitialized data.
> 
> 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>

Could you add:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

And combine the cover letter with the commit message?
For a single-patch postings cover letter is not necessary.

> +		*(__be16 *)skb_push(skb, 2) = htons(PPP_FILTER_OUTBOUND_TAG);
>  		if (ppp->pass_filter &&
>  		    bpf_prog_run(ppp->pass_filter, skb) == 0) {
>  			if (ppp->debug & 1)

The exact same problem seems to be present in ppp_receive_nonmp_frame()
please fix them both.
-- 
pw-bot: cr

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

[Jiayuan Chen]

On Thu, Feb 27, 2025 at 05:48:12PM -0800, Jakub Kicinski wrote:
> On Wed, 26 Feb 2025 09:36:58 +0800 Jiayuan Chen wrote:
> > The PPP driver adds an extra 2-byte header to enable socket filters to run
> Could you add:
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> And combine the cover letter with the commit message?
> For a single-patch postings cover letter is not necessary.
> 
> > +		*(__be16 *)skb_push(skb, 2) = htons(PPP_FILTER_OUTBOUND_TAG);
> >  		if (ppp->pass_filter &&
> >  		    bpf_prog_run(ppp->pass_filter, skb) == 0) {
> >  			if (ppp->debug & 1)
> 
> The exact same problem seems to be present in ppp_receive_nonmp_frame()
> please fix them both.
> -- 
> pw-bot: cr
Thanks, Jakub! I'll do that.

I was previously worried that the commit message would be too long,
so I put the detailed information in the cover letter instead. I'll make
the commit message more concise.

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

[Jakub Kicinski]

On Fri, 28 Feb 2025 12:55:41 +0800 Jiayuan Chen wrote:
> > The exact same problem seems to be present in ppp_receive_nonmp_frame()
> > please fix them both.
> > -- 
> > pw-bot: cr  
> Thanks, Jakub! I'll do that.
> 
> I was previously worried that the commit message would be too long,
> so I put the detailed information in the cover letter instead. I'll make
> the commit message more concise.

FWIW we commit the cover letter as well, as a merge commit.
So both would end up in the tree.