Re: [PATCH net v6 2/2] pppoe: drop PFC frames

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
pppoe: drop PFC frames

This commit drops Protocol Field Compression (PFC) frames for PPPoE to avoid
unaligned access exceptions caused by the network header being shifted. A
helper function is introduced to identify compressed protocol fields.

> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
[ ... ]
> @@ -434,6 +434,12 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (skb->len < len)
>  		goto drop;
>  
> +	/* skb->data points to the PPP protocol header after skb_pull_rcsum.
> +	 * Drop PFC frames.
> +	 */
> +	if (ppp_skb_is_compressed_proto(skb))

Does this code safely access the PPP protocol byte?

After pulling the PPPoE header, skb->data points to the inner payload.
However, the new ppp_skb_is_compressed_proto() helper unconditionally
dereferences skb->data[0].

If a crafted PPPoE packet is received with an inner length of 0 and no
Ethernet padding, the linear buffer might end exactly after the PPPoE header.
In that scenario, this would read past the allocated linear buffer.

Even if the inner length is greater than 0, could the payload reside entirely
in non-linear paged fragments, causing an out-of-bounds read?

Would it be safer to verify the packet has at least 1 byte and use
pskb_may_pull() to ensure the protocol byte is in the linear region before
inspecting it, perhaps after the pskb_trim_rcsum() call?

> +		goto drop;
> +
>  	if (pskb_trim_rcsum(skb, len))
>  		goto drop;
[ ... ]
> diff --git a/include/linux/ppp_defs.h b/include/linux/ppp_defs.h
> --- a/include/linux/ppp_defs.h
> +++ b/include/linux/ppp_defs.h
[ ... ]
> +static inline bool ppp_skb_is_compressed_proto(const struct sk_buff *skb)
> +{
> +	return unlikely(skb->data[0] & 0x01);
> +}
-- 
pw-bot: cr