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

[PATCH net-next v2 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:

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:
'''
tcpdump -d ip -y PPP
(000) ldh      [2]
(001) jeq      #0x21            jt 2	jf 3
(002) ret      #262144
(003) ret
'''

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.

In the current implementation, 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

---
v1 -> v2:
https://lore.kernel.org/linux-ppp/20250218133145.265313-1-jiayuan.chen@linux.dev/T/#t
Add more comments.
Correctly initialize on big-endian and little-endian systems.
---

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

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

-- 
2.47.1

[PATCH net-next v2 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 generated by libpcap, 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 | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4583e15ad03a..ac95196c85df 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1762,10 +1762,16 @@ 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.
+		 * BPF filter instructions assume each PPP packet has a 4-byte
+		 * header (e.g., those generated by libpcap), and then default
+		 * to skipping the first 2 bytes at the beginning of the
+		 * instruction. However, we still need to initialize these
+		 * 2-byte new headers to prevent crafted BPF programs from
+		 * reading them which would cause reading of uninitialized
+		 * data. Here, we set the headers according to the RFC 1662.
+		 */
+		*(u16 *)skb_push(skb, 2) = htons(0xff03);
 		if (ppp->pass_filter &&
 		    bpf_prog_run(ppp->pass_filter, skb) == 0) {
 			if (ppp->debug & 1)
-- 
2.47.1

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

[Jakub Kicinski]

On Fri, 21 Feb 2025 14:12:19 +0800 Jiayuan Chen wrote:
> +		/* Check if we should pass this packet.
> +		 * BPF filter instructions assume each PPP packet has a 4-byte
> +		 * header (e.g., those generated by libpcap), and then default
> +		 * to skipping the first 2 bytes at the beginning of the
> +		 * instruction. However, we still need to initialize these
> +		 * 2-byte new headers to prevent crafted BPF programs from
> +		 * reading them which would cause reading of uninitialized
> +		 * data. Here, we set the headers according to the RFC 1662.
> +		 */
> +		*(u16 *)skb_push(skb, 2) = htons(0xff03);

The constant from the RFC deserves a #define or enum.
Looks like we may already need it in one other place:

drivers/net/wan/fsl_ucc_hdlc.h:#define DEFAULT_PPP_HEAD    0xff03
-- 
pw-bot: cr

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

[Jiayuan Chen]

On Mon, Feb 24, 2025 at 02:26:44PM -0800, Jakub Kicinski wrote:
> On Fri, 21 Feb 2025 14:12:19 +0800 Jiayuan Chen wrote:
> > +		/* Check if we should pass this packet.
> > +		 * BPF filter instructions assume each PPP packet has a 4-byte
> > +		 * header (e.g., those generated by libpcap), and then default
> > +		 * to skipping the first 2 bytes at the beginning of the
> > +		 * instruction. However, we still need to initialize these
> > +		 * 2-byte new headers to prevent crafted BPF programs from
> > +		 * reading them which would cause reading of uninitialized
> > +		 * data. Here, we set the headers according to the RFC 1662.
> > +		 */
> > +		*(u16 *)skb_push(skb, 2) = htons(0xff03);
> 
> The constant from the RFC deserves a #define or enum.
> Looks like we may already need it in one other place:
> 
> drivers/net/wan/fsl_ucc_hdlc.h:#define DEFAULT_PPP_HEAD    0xff03
> -- 
> pw-bot: cr
Hi Jakub,

I apologize for the mistake, I've investigated the original maintainer's
user-space PPP implementation and libpcap's behavior, and found that
initializing the first byte to 0 or 1 is necessary, it indicates
direction, which is used in libpcap to distinguish between inbound and
outbound traffic.
For more details, please refer to the cover letter of my v3 patch.

https://lore.kernel.org/linux-ppp/20250222092556.274267-1-jiayuan.chen@linux.dev/T/#t

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

[Jakub Kicinski]

On Tue, 25 Feb 2025 07:20:08 +0800 Jiayuan Chen wrote:
> I apologize for the mistake, I've investigated the original maintainer's
> user-space PPP implementation and libpcap's behavior, and found that
> initializing the first byte to 0 or 1 is necessary, it indicates
> direction, which is used in libpcap to distinguish between inbound and
> outbound traffic.
> For more details, please refer to the cover letter of my v3 patch.
> 
> https://lore.kernel.org/linux-ppp/20250222092556.274267-1-jiayuan.chen@linux.dev/T/#t

You can still use the htons() there, it's cleaner.