Two missing check in virtio_net_hdr_to_skb() allowed syzbot
to crash kernels again
1. After the skb_segment function the buffer may become non-linear
(nr_frags != 0), but since the SKBTX_SHARED_FRAG flag is not set anywhere
the __skb_linearize function will not be executed, then the buffer will
remain non-linear. Then the condition (offset >= skb_headlen(skb))
becomes true, which causes WARN_ON_ONCE in skb_checksum_help.
2. The struct sk_buff and struct virtio_net_hdr members must be
mathematically related.
(gso_size) must be greater than (needed) otherwise WARN_ON_ONCE.
(remainder) must be greater than (needed) otherwise WARN_ON_ONCE.
(remainder) may be 0 if division is without remainder.
offset+2 (4191) > skb_headlen() (1116)
WARNING: CPU: 1 PID: 5084 at net/core/dev.c:3303 skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
Modules linked in:
CPU: 1 PID: 5084 Comm: syz-executor336 Not tainted 6.7.0-rc3-syzkaller-00014-gdf60cee26a2e #0
Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
RIP: 0010:skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
Code: 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 52 01 00 00 44 89 e2 2b 53 74 4c 89 ee 48 c7 c7 40 57 e9 8b e8 af 8f dd f8 90 <0f> 0b 90 90 e9 87 fe ff ff e8 40 0f 6e f9 e9 4b fa ff ff 48 89 ef
RSP: 0018:ffffc90003a9f338 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff888025125780 RCX: ffffffff814db209
RDX: ffff888015393b80 RSI: ffffffff814db216 RDI: 0000000000000001
RBP: ffff8880251257f4 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: 000000000000045c
R13: 000000000000105f R14: ffff8880251257f0 R15: 000000000000105d
FS: 0000555555c24380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000002000f000 CR3: 0000000023151000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
ip_do_fragment+0xa1b/0x18b0 net/ipv4/ip_output.c:777
ip_fragment.constprop.0+0x161/0x230 net/ipv4/ip_output.c:584
ip_finish_output_gso net/ipv4/ip_output.c:286 [inline]
__ip_finish_output net/ipv4/ip_output.c:308 [inline]
__ip_finish_output+0x49c/0x650 net/ipv4/ip_output.c:295
ip_finish_output+0x31/0x310 net/ipv4/ip_output.c:323
NF_HOOK_COND include/linux/netfilter.h:303 [inline]
ip_output+0x13b/0x2a0 net/ipv4/ip_output.c:433
dst_output include/net/dst.h:451 [inline]
ip_local_out+0xaf/0x1a0 net/ipv4/ip_output.c:129
iptunnel_xmit+0x5b4/0x9b0 net/ipv4/ip_tunnel_core.c:82
ipip6_tunnel_xmit net/ipv6/sit.c:1034 [inline]
sit_tunnel_xmit+0xed2/0x28f0 net/ipv6/sit.c:1076
__netdev_start_xmit include/linux/netdevice.h:4940 [inline]
netdev_start_xmit include/linux/netdevice.h:4954 [inline]
xmit_one net/core/dev.c:3545 [inline]
dev_hard_start_xmit+0x13d/0x6d0 net/core/dev.c:3561
__dev_queue_xmit+0x7c1/0x3d60 net/core/dev.c:4346
dev_queue_xmit include/linux/netdevice.h:3134 [inline]
packet_xmit+0x257/0x380 net/packet/af_packet.c:276
packet_snd net/packet/af_packet.c:3087 [inline]
packet_sendmsg+0x24ca/0x5240 net/packet/af_packet.c:3119
sock_sendmsg_nosec net/socket.c:730 [inline]
__sock_sendmsg+0xd5/0x180 net/socket.c:745
__sys_sendto+0x255/0x340 net/socket.c:2190
__do_sys_sendto net/socket.c:2202 [inline]
__se_sys_sendto net/socket.c:2198 [inline]
__x64_sys_sendto+0xe0/0x1b0 net/socket.c:2198
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
Signed-off-by: Denis Arefev <[email protected]>
---
include/linux/virtio_net.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 4dfa9b69ca8d..77ebe908d746 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -56,6 +56,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
unsigned int thlen = 0;
unsigned int p_off = 0;
unsigned int ip_proto;
+ u64 ret, remainder;
if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -98,6 +99,15 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
+ if (hdr->gso_size) {
+ ret = div64_u64_rem(skb->len, hdr->gso_size, &remainder);
+ if (!(ret && (hdr->gso_size > needed) &&
+ ((remainder > needed) || (remainder == 0)))) {
+ return -EINVAL;
+ }
+ skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
+ }
+
if (!pskb_may_pull(skb, needed))
return -EINVAL;
--
2.25.1
On Thu, Jun 6, 2024 at 4:14 PM Denis Arefev <[email protected]> wrote:
>
> Two missing check in virtio_net_hdr_to_skb() allowed syzbot
> to crash kernels again
>
> 1. After the skb_segment function the buffer may become non-linear
> (nr_frags != 0), but since the SKBTX_SHARED_FRAG flag is not set anywhere
> the __skb_linearize function will not be executed, then the buffer will
> remain non-linear. Then the condition (offset >= skb_headlen(skb))
> becomes true, which causes WARN_ON_ONCE in skb_checksum_help.
>
> 2. The struct sk_buff and struct virtio_net_hdr members must be
> mathematically related.
> (gso_size) must be greater than (needed) otherwise WARN_ON_ONCE.
> (remainder) must be greater than (needed) otherwise WARN_ON_ONCE.
> (remainder) may be 0 if division is without remainder.
>
> offset+2 (4191) > skb_headlen() (1116)
> WARNING: CPU: 1 PID: 5084 at net/core/dev.c:3303 skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
> Modules linked in:
> CPU: 1 PID: 5084 Comm: syz-executor336 Not tainted 6.7.0-rc3-syzkaller-00014-gdf60cee26a2e #0
> Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
> RIP: 0010:skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
> Code: 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 52 01 00 00 44 89 e2 2b 53 74 4c 89 ee 48 c7 c7 40 57 e9 8b e8 af 8f dd f8 90 <0f> 0b 90 90 e9 87 fe ff ff e8 40 0f 6e f9 e9 4b fa ff ff 48 89 ef
> RSP: 0018:ffffc90003a9f338 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: ffff888025125780 RCX: ffffffff814db209
> RDX: ffff888015393b80 RSI: ffffffff814db216 RDI: 0000000000000001
> RBP: ffff8880251257f4 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000001 R12: 000000000000045c
> R13: 000000000000105f R14: ffff8880251257f0 R15: 000000000000105d
> FS: 0000555555c24380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000002000f000 CR3: 0000000023151000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ip_do_fragment+0xa1b/0x18b0 net/ipv4/ip_output.c:777
> ip_fragment.constprop.0+0x161/0x230 net/ipv4/ip_output.c:584
> ip_finish_output_gso net/ipv4/ip_output.c:286 [inline]
> __ip_finish_output net/ipv4/ip_output.c:308 [inline]
> __ip_finish_output+0x49c/0x650 net/ipv4/ip_output.c:295
> ip_finish_output+0x31/0x310 net/ipv4/ip_output.c:323
> NF_HOOK_COND include/linux/netfilter.h:303 [inline]
> ip_output+0x13b/0x2a0 net/ipv4/ip_output.c:433
> dst_output include/net/dst.h:451 [inline]
> ip_local_out+0xaf/0x1a0 net/ipv4/ip_output.c:129
> iptunnel_xmit+0x5b4/0x9b0 net/ipv4/ip_tunnel_core.c:82
> ipip6_tunnel_xmit net/ipv6/sit.c:1034 [inline]
> sit_tunnel_xmit+0xed2/0x28f0 net/ipv6/sit.c:1076
> __netdev_start_xmit include/linux/netdevice.h:4940 [inline]
> netdev_start_xmit include/linux/netdevice.h:4954 [inline]
> xmit_one net/core/dev.c:3545 [inline]
> dev_hard_start_xmit+0x13d/0x6d0 net/core/dev.c:3561
> __dev_queue_xmit+0x7c1/0x3d60 net/core/dev.c:4346
> dev_queue_xmit include/linux/netdevice.h:3134 [inline]
> packet_xmit+0x257/0x380 net/packet/af_packet.c:276
> packet_snd net/packet/af_packet.c:3087 [inline]
> packet_sendmsg+0x24ca/0x5240 net/packet/af_packet.c:3119
> sock_sendmsg_nosec net/socket.c:730 [inline]
> __sock_sendmsg+0xd5/0x180 net/socket.c:745
> __sys_sendto+0x255/0x340 net/socket.c:2190
> __do_sys_sendto net/socket.c:2202 [inline]
> __se_sys_sendto net/socket.c:2198 [inline]
> __x64_sys_sendto+0xe0/0x1b0 net/socket.c:2198
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> Signed-off-by: Denis Arefev <[email protected]>
> ---
> include/linux/virtio_net.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 4dfa9b69ca8d..77ebe908d746 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -56,6 +56,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> unsigned int thlen = 0;
> unsigned int p_off = 0;
> unsigned int ip_proto;
> + u64 ret, remainder;
>
> if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> @@ -98,6 +99,15 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
>
> + if (hdr->gso_size) {
> + ret = div64_u64_rem(skb->len, hdr->gso_size, &remainder);
> + if (!(ret && (hdr->gso_size > needed) &&
> + ((remainder > needed) || (remainder == 0)))) {
> + return -EINVAL;
> + }
> + skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
> + }
> +
> if (!pskb_may_pull(skb, needed))
> return -EINVAL;
>
Hi Denis
Please please please cc netdev@ for this kind of patch.
gso_size has no relation to @needed.
I doubt div64_u64_rem() is needed.
SKBFL_SHARED_FRAG should not be abused like that.
Bug must be elsewhere. Do you have a repro ?
I think syzbot has a similar report.
Hi Denis,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc2 next-20240607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Denis-Arefev/net-missing-check/20240606-230540
base: linus/master
patch link: https://lore.kernel.org/r/20240606141450.44709-1-arefev%40swemel.ru
patch subject: [PATCH] net: missing check
config: x86_64-randconfig-121-20240607 (https://download.01.org/0day-ci/archive/20240607/[email protected]/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
drivers/net/virtio_net.c: note: in included file:
>> include/linux/virtio_net.h:103:58: sparse: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned long long [usertype] divisor @@ got restricted __virtio16 const [usertype] gso_size @@
include/linux/virtio_net.h:103:58: sparse: expected unsigned long long [usertype] divisor
include/linux/virtio_net.h:103:58: sparse: got restricted __virtio16 const [usertype] gso_size
>> include/linux/virtio_net.h:104:42: sparse: sparse: restricted __virtio16 degrades to integer
vim +103 include/linux/virtio_net.h
49
50 static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
51 const struct virtio_net_hdr *hdr,
52 bool little_endian)
53 {
54 unsigned int nh_min_len = sizeof(struct iphdr);
55 unsigned int gso_type = 0;
56 unsigned int thlen = 0;
57 unsigned int p_off = 0;
58 unsigned int ip_proto;
59 u64 ret, remainder;
60
61 if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
62 switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
63 case VIRTIO_NET_HDR_GSO_TCPV4:
64 gso_type = SKB_GSO_TCPV4;
65 ip_proto = IPPROTO_TCP;
66 thlen = sizeof(struct tcphdr);
67 break;
68 case VIRTIO_NET_HDR_GSO_TCPV6:
69 gso_type = SKB_GSO_TCPV6;
70 ip_proto = IPPROTO_TCP;
71 thlen = sizeof(struct tcphdr);
72 nh_min_len = sizeof(struct ipv6hdr);
73 break;
74 case VIRTIO_NET_HDR_GSO_UDP:
75 gso_type = SKB_GSO_UDP;
76 ip_proto = IPPROTO_UDP;
77 thlen = sizeof(struct udphdr);
78 break;
79 case VIRTIO_NET_HDR_GSO_UDP_L4:
80 gso_type = SKB_GSO_UDP_L4;
81 ip_proto = IPPROTO_UDP;
82 thlen = sizeof(struct udphdr);
83 break;
84 default:
85 return -EINVAL;
86 }
87
88 if (hdr->gso_type & VIRTIO_NET_HDR_GSO_ECN)
89 gso_type |= SKB_GSO_TCP_ECN;
90
91 if (hdr->gso_size == 0)
92 return -EINVAL;
93 }
94
95 skb_reset_mac_header(skb);
96
97 if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
98 u32 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
99 u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
100 u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
101
102 if (hdr->gso_size) {
> 103 ret = div64_u64_rem(skb->len, hdr->gso_size, &remainder);
> 104 if (!(ret && (hdr->gso_size > needed) &&
105 ((remainder > needed) || (remainder == 0)))) {
106 return -EINVAL;
107 }
108 skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
109 }
110
111 if (!pskb_may_pull(skb, needed))
112 return -EINVAL;
113
114 if (!skb_partial_csum_set(skb, start, off))
115 return -EINVAL;
116
117 nh_min_len = max_t(u32, nh_min_len, skb_transport_offset(skb));
118 p_off = nh_min_len + thlen;
119 if (!pskb_may_pull(skb, p_off))
120 return -EINVAL;
121 } else {
122 /* gso packets without NEEDS_CSUM do not set transport_offset.
123 * probe and drop if does not match one of the above types.
124 */
125 if (gso_type && skb->network_header) {
126 struct flow_keys_basic keys;
127
128 if (!skb->protocol) {
129 __be16 protocol = dev_parse_header_protocol(skb);
130
131 if (!protocol)
132 virtio_net_hdr_set_proto(skb, hdr);
133 else if (!virtio_net_hdr_match_proto(protocol, hdr->gso_type))
134 return -EINVAL;
135 else
136 skb->protocol = protocol;
137 }
138 retry:
139 if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
140 NULL, 0, 0, 0,
141 0)) {
142 /* UFO does not specify ipv4 or 6: try both */
143 if (gso_type & SKB_GSO_UDP &&
144 skb->protocol == htons(ETH_P_IP)) {
145 skb->protocol = htons(ETH_P_IPV6);
146 goto retry;
147 }
148 return -EINVAL;
149 }
150
151 p_off = keys.control.thoff + thlen;
152 if (!pskb_may_pull(skb, p_off) ||
153 keys.basic.ip_proto != ip_proto)
154 return -EINVAL;
155
156 skb_set_transport_header(skb, keys.control.thoff);
157 } else if (gso_type) {
158 p_off = nh_min_len + thlen;
159 if (!pskb_may_pull(skb, p_off))
160 return -EINVAL;
161 }
162 }
163
164 if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
165 u16 gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
166 unsigned int nh_off = p_off;
167 struct skb_shared_info *shinfo = skb_shinfo(skb);
168
169 switch (gso_type & ~SKB_GSO_TCP_ECN) {
170 case SKB_GSO_UDP:
171 /* UFO may not include transport header in gso_size. */
172 nh_off -= thlen;
173 break;
174 case SKB_GSO_UDP_L4:
175 if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
176 return -EINVAL;
177 if (skb->csum_offset != offsetof(struct udphdr, check))
178 return -EINVAL;
179 if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
180 return -EINVAL;
181 if (gso_type != SKB_GSO_UDP_L4)
182 return -EINVAL;
183 break;
184 }
185
186 /* Kernel has a special handling for GSO_BY_FRAGS. */
187 if (gso_size == GSO_BY_FRAGS)
188 return -EINVAL;
189
190 /* Too small packets are not really GSO ones. */
191 if (skb->len - nh_off > gso_size) {
192 shinfo->gso_size = gso_size;
193 shinfo->gso_type = gso_type;
194
195 /* Header must be checked, and gso_segs computed. */
196 shinfo->gso_type |= SKB_GSO_DODGY;
197 shinfo->gso_segs = 0;
198 }
199 }
200
201 return 0;
202 }
203
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki