Re: [PATCH v2 net] dccp: fix dccp_v4_err()/dccp_v6_err() again
flat view
From: Jann Horn <jannh@google.com>
Date: 2023-09-15 19:14:32
On Fri, Sep 15, 2023 at 9:00 PM Eric Dumazet [off-list ref] 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 <redacted>
Signed-off-by: Eric Dumazet <redacted>
Cc: Jann Horn <jannh@google.com>Reviewed-by: Jann Horn <jannh@google.com>
quoted hunk ↗ jump to hunk
--- 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