netdev - Re: [PATCH net] llc: verify mac len before reading mac header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iKr52cVenLBYcOAdOKQiN+J1-gtwfnwuRuauJC0fJgFLQ@mail.gmail.com>
Date: Wed, 25 Oct 2023 09:09:06 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, Willem de Bruijn <willemb@...gle.com>, 
	syzbot+a8c7be6dee0de1b669cc@...kaller.appspotmail.com
Subject: Re: [PATCH net] llc: verify mac len before reading mac header

On Tue, Oct 24, 2023 at 9:50 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> From: Willem de Bruijn <willemb@...gle.com>
>
> LLC reads the mac header with eth_hdr without verifying that the skb
> has an Ethernet header.
>
> Syzbot was able to enter llc_rcv on a tun device. Tun can insert
> packets without mac len and with user configurable skb->protocol
> (passing a tun_pi header when not configuring IFF_NO_PI).
>
>     BUG: KMSAN: uninit-value in llc_station_ac_send_test_r net/llc/llc_station.c:81 [inline]
>     BUG: KMSAN: uninit-value in llc_station_rcv+0x6fb/0x1290 net/llc/llc_station.c:111
>     llc_station_ac_send_test_r net/llc/llc_station.c:81 [inline]
>     llc_station_rcv+0x6fb/0x1290 net/llc/llc_station.c:111
>     llc_rcv+0xc5d/0x14a0 net/llc/llc_input.c:218
>     __netif_receive_skb_one_core net/core/dev.c:5523 [inline]
>     __netif_receive_skb+0x1a6/0x5a0 net/core/dev.c:5637
>     netif_receive_skb_internal net/core/dev.c:5723 [inline]
>     netif_receive_skb+0x58/0x660 net/core/dev.c:5782
>     tun_rx_batched+0x3ee/0x980 drivers/net/tun.c:1555
>     tun_get_user+0x54c5/0x69c0 drivers/net/tun.c:2002
>
> Add a mac_len test before all three eth_hdr(skb) calls under net/llc.
>
> There are further uses in include/net/llc_pdu.h. All these are
> protected by a test skb->protocol == ETH_P_802_2. Which does not
> protect against this tun scenario.
>
> But the mac_len test added in this patch in llc_fixup_skb will
> indirectly protect those too. That is called from llc_rcv before any
> other LLC code.
>
> It is tempting to just add a blanket mac_len check in llc_rcv, but
> not sure whether that could break valid LLC paths that do not assume
> an Ethernet header. 802.2 LLC may be used on top of non-802.3
> protocols in principle.
>
> Reported-by: syzbot+a8c7be6dee0de1b669cc@...kaller.appspotmail.com

Note that the syzbot report is still private in syzbot infra.

Reviewed-by: Eric Dumazet <edumazet@...gle.com>

> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ---
>  net/llc/llc_input.c   | 10 ++++++++--
>  net/llc/llc_s_ac.c    |  3 +++
>  net/llc/llc_station.c |  3 +++
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c
> index 7cac441862e21..51bccfb00a9cd 100644
> --- a/net/llc/llc_input.c
> +++ b/net/llc/llc_input.c
> @@ -127,8 +127,14 @@ static inline int llc_fixup_skb(struct sk_buff *skb)
>         skb->transport_header += llc_len;
>         skb_pull(skb, llc_len);
>         if (skb->protocol == htons(ETH_P_802_2)) {
> -               __be16 pdulen = eth_hdr(skb)->h_proto;
> -               s32 data_size = ntohs(pdulen) - llc_len;
> +               __be16 pdulen;
> +               s32 data_size;
> +
> +               if (skb->mac_len < ETH_HLEN)
> +                       return 0;
> +
> +               pdulen = eth_hdr(skb)->h_proto;
> +               data_size = ntohs(pdulen) - llc_len;
>
>                 if (data_size < 0 ||
>                     !pskb_may_pull(skb, data_size))
> diff --git a/net/llc/llc_s_ac.c b/net/llc/llc_s_ac.c
> index 79d1cef8f15a9..7923c064773cc 100644
> --- a/net/llc/llc_s_ac.c
> +++ b/net/llc/llc_s_ac.c
> @@ -153,6 +153,9 @@ int llc_sap_action_send_test_r(struct llc_sap *sap, struct sk_buff *skb)
>         int rc = 1;
>         u32 data_size;
>
> +       if (skb->mac_len < ETH_HLEN)
> +               return 0;
> +
>         llc_pdu_decode_sa(skb, mac_da);
>         llc_pdu_decode_da(skb, mac_sa);
>         llc_pdu_decode_ssap(skb, &dsap);
> diff --git a/net/llc/llc_station.c b/net/llc/llc_station.c
> index 05c6ae0920534..f506542925109 100644
> --- a/net/llc/llc_station.c
> +++ b/net/llc/llc_station.c
> @@ -76,6 +76,9 @@ static int llc_station_ac_send_test_r(struct sk_buff *skb)
>         u32 data_size;
>         struct sk_buff *nskb;
>
> +       if (skb->mac_len < ETH_HLEN)
> +               goto out;
> +
>         /* The test request command is type U (llc_len = 3) */
>         data_size = ntohs(eth_hdr(skb)->h_proto) - 3;
>         nskb = llc_alloc_frame(NULL, skb->dev, LLC_PDU_TYPE_U, data_size);
> --
> 2.42.0.758.gaed0368e0e-goog
>