netdev - Re: [PATCH v2 net] dccp: fix dccp_v4_err()/dccp_v6

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: <CAG48ez2PsxR9uqP+8XOeF8b_VZqXS9tb7Qqjzz5L-573CUFuAQ@mail.gmail.com>
Date: Fri, 15 Sep 2023 21:13:50 +0200
From: Jann Horn <jannh@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH v2 net] dccp: fix dccp_v4_err()/dccp_v6_err() again

On Fri, Sep 15, 2023 at 9:00 PM Eric Dumazet <edumazet@...gle.com> wrote:
> dh->dccph_x is the 9th byte (offset 8) in "struct dccp_hdr",
> not in the "byte 7" as Jann claimed.
>
> We need to make sure the ICMP messages are big enough,
> using more standard ways (no more assumptions).
>
> syzbot reported:
> BUG: KMSAN: uninit-value in pskb_may_pull_reason include/linux/skbuff.h:2667 [inline]
> BUG: KMSAN: uninit-value in pskb_may_pull include/linux/skbuff.h:2681 [inline]
> BUG: KMSAN: uninit-value in dccp_v6_err+0x426/0x1aa0 net/dccp/ipv6.c:94

By the way, I didn't realize syzbot could catch SKB head OOB reads
thanks to KMSAN. Neat!

[...]
> CPU: 0 PID: 4995 Comm: syz-executor153 Not tainted 6.6.0-rc1-syzkaller-00014-ga747acc0b752 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/04/2023
>
> Fixes: 977ad86c2a1b ("dccp: Fix out of bounds access in DCCP error handler")
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Jann Horn <jannh@...gle.com>

Reviewed-by: Jann Horn <jannh@...gle.com>

> ---
> v2: fix a typo I made on Jann name, sorry !
>  net/dccp/ipv4.c | 9 ++-------
>  net/dccp/ipv6.c | 9 ++-------
>  2 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
> index 8f56e8723c7386c9f9344f1376823bfd0077c8c2..69453b936bd557c77a790a27ff64cc91e5a58296 100644
> --- a/net/dccp/ipv4.c
> +++ b/net/dccp/ipv4.c
> @@ -254,13 +254,8 @@ static int dccp_v4_err(struct sk_buff *skb, u32 info)
>         int err;
>         struct net *net = dev_net(skb->dev);
>
> -       /* For the first __dccp_basic_hdr_len() check, we only need dh->dccph_x,
> -        * which is in byte 7 of the dccp header.
> -        * Our caller (icmp_socket_deliver()) already pulled 8 bytes for us.
> -        *
> -        * Later on, we want to access the sequence number fields, which are
> -        * beyond 8 bytes, so we have to pskb_may_pull() ourselves.
> -        */
> +       if (!pskb_may_pull(skb, offset + sizeof(*dh)))
> +               return -EINVAL;
>         dh = (struct dccp_hdr *)(skb->data + offset);
>         if (!pskb_may_pull(skb, offset + __dccp_basic_hdr_len(dh)))
>                 return -EINVAL;
> diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
> index 33f6ccf6ba77b9bcc24054b09857aaee4bb71acf..c693a570682fba2ad93c7bceb8788bd9d51a0b41 100644
> --- a/net/dccp/ipv6.c
> +++ b/net/dccp/ipv6.c
> @@ -83,13 +83,8 @@ static int dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>         __u64 seq;
>         struct net *net = dev_net(skb->dev);
>
> -       /* For the first __dccp_basic_hdr_len() check, we only need dh->dccph_x,
> -        * which is in byte 7 of the dccp header.
> -        * Our caller (icmpv6_notify()) already pulled 8 bytes for us.
> -        *
> -        * Later on, we want to access the sequence number fields, which are
> -        * beyond 8 bytes, so we have to pskb_may_pull() ourselves.
> -        */
> +       if (!pskb_may_pull(skb, offset + sizeof(*dh)))
> +               return -EINVAL;
>         dh = (struct dccp_hdr *)(skb->data + offset);
>         if (!pskb_may_pull(skb, offset + __dccp_basic_hdr_len(dh)))
>                 return -EINVAL;
> --
> 2.42.0.459.ge4e396fd5e-goog
>