net/core/skbuff.c | 5 +++++ 1 file changed, 5 insertions(+)
pskb_expand_head doesn't initialize extra nhead bytes in header and
tail bytes, leading to KMSAN infoleak error. Fix by initializing data to
0 with memset.
Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
Tested-by: Daniel Yang <danielyangkang@gmail.com>
Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
---
net/core/skbuff.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 74149dc4e..348161dcb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2286,6 +2286,11 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
skb_shinfo(skb),
offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
+ /* Initialize newly allocated headroom and tailroom
+ */
+ memset(data, 0, nhead);
+ memset(data + nhead + skb->tail, 0, skb_tailroom(skb) + ntail);
+
/*
* if shinfo is shared we must drop the old head gracefully, but if it
* is not we can just drop the old head and let the existing refcount
--
2.39.2Hi Daniel,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on horms-ipvs/master v6.12-rc1 next-20241004]
[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/Daniel-Yang/Fix-KMSAN-infoleak-initialize-unused-data-in-pskb_expand_head/20241002-134054
base: linus/master
patch link: https://lore.kernel.org/r/20241002053844.130553-1-danielyangkang%40gmail.com
patch subject: [PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head
config: powerpc-randconfig-r071-20241004 (https://download.01.org/0day-ci/archive/20241004/202410041506.hEO3Ubsh-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410041506.hEO3Ubsh-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410041506.hEO3Ubsh-lkp@intel.com/
All errors (new ones prefixed by >>):
net/core/skbuff.c: In function 'pskb_expand_head':
>> net/core/skbuff.c:2292:29: error: invalid operands to binary + (have 'u8 *' {aka 'unsigned char *'} and 'sk_buff_data_t' {aka 'unsigned char *'})
2292 | memset(data + nhead + skb->tail, 0, skb_tailroom(skb) + ntail);
| ~~~~~~~~~~~~ ^ ~~~~~~~~~
| | |
| | sk_buff_data_t {aka unsigned char *}
| u8 * {aka unsigned char *}
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [y]:
- RESOURCE_KUNIT_TEST [=y] && RUNTIME_TESTING_MENU [=y] && KUNIT [=y]
vim +2292 net/core/skbuff.c
2240
2241 /**
2242 * pskb_expand_head - reallocate header of &sk_buff
2243 * @skb: buffer to reallocate
2244 * @nhead: room to add at head
2245 * @ntail: room to add at tail
2246 * @gfp_mask: allocation priority
2247 *
2248 * Expands (or creates identical copy, if @nhead and @ntail are zero)
2249 * header of @skb. &sk_buff itself is not changed. &sk_buff MUST have
2250 * reference count of 1. Returns zero in the case of success or error,
2251 * if expansion failed. In the last case, &sk_buff is not changed.
2252 *
2253 * All the pointers pointing into skb header may change and must be
2254 * reloaded after call to this function.
2255 */
2256
2257 int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
2258 gfp_t gfp_mask)
2259 {
2260 unsigned int osize = skb_end_offset(skb);
2261 unsigned int size = osize + nhead + ntail;
2262 long off;
2263 u8 *data;
2264 int i;
2265
2266 BUG_ON(nhead < 0);
2267
2268 BUG_ON(skb_shared(skb));
2269
2270 skb_zcopy_downgrade_managed(skb);
2271
2272 if (skb_pfmemalloc(skb))
2273 gfp_mask |= __GFP_MEMALLOC;
2274
2275 data = kmalloc_reserve(&size, gfp_mask, NUMA_NO_NODE, NULL);
2276 if (!data)
2277 goto nodata;
2278 size = SKB_WITH_OVERHEAD(size);
2279
2280 /* Copy only real data... and, alas, header. This should be
2281 * optimized for the cases when header is void.
2282 */
2283 memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head);
2284
2285 memcpy((struct skb_shared_info *)(data + size),
2286 skb_shinfo(skb),
2287 offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
2288
2289 /* Initialize newly allocated headroom and tailroom
2290 */
2291 memset(data, 0, nhead);
> 2292 memset(data + nhead + skb->tail, 0, skb_tailroom(skb) + ntail);
2293
2294 /*
2295 * if shinfo is shared we must drop the old head gracefully, but if it
2296 * is not we can just drop the old head and let the existing refcount
2297 * be since all we did is relocate the values
2298 */
2299 if (skb_cloned(skb)) {
2300 if (skb_orphan_frags(skb, gfp_mask))
2301 goto nofrags;
2302 if (skb_zcopy(skb))
2303 refcount_inc(&skb_uarg(skb)->refcnt);
2304 for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
2305 skb_frag_ref(skb, i);
2306
2307 if (skb_has_frag_list(skb))
2308 skb_clone_fraglist(skb);
2309
2310 skb_release_data(skb, SKB_CONSUMED);
2311 } else {
2312 skb_free_head(skb);
2313 }
2314 off = (data + nhead) - skb->head;
2315
2316 skb->head = data;
2317 skb->head_frag = 0;
2318 skb->data += off;
2319
2320 skb_set_end_offset(skb, size);
2321 #ifdef NET_SKBUFF_DATA_USES_OFFSET
2322 off = nhead;
2323 #endif
2324 skb->tail += off;
2325 skb_headers_offset_update(skb, nhead);
2326 skb->cloned = 0;
2327 skb->hdr_len = 0;
2328 skb->nohdr = 0;
2329 atomic_set(&skb_shinfo(skb)->dataref, 1);
2330
2331 skb_metadata_clear(skb);
2332
2333 /* It is not generally safe to change skb->truesize.
2334 * For the moment, we really care of rx path, or
2335 * when skb is orphaned (not attached to a socket).
2336 */
2337 if (!skb->sk || skb->destructor == sock_edemux)
2338 skb->truesize += size - osize;
2339
2340 return 0;
2341
2342 nofrags:
2343 skb_kfree_head(data, size);
2344 nodata:
2345 return -ENOMEM;
2346 }
2347 EXPORT_SYMBOL(pskb_expand_head);
2348
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Oct 2, 2024 at 7:39 AM Daniel Yang <danielyangkang@gmail.com> wrote:
>
> pskb_expand_head doesn't initialize extra nhead bytes in header and
> tail bytes, leading to KMSAN infoleak error. Fix by initializing data to
> 0 with memset.
>
> Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
> Tested-by: Daniel Yang <danielyangkang@gmail.com>
> Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
No no no.
Please fix the root cause, instead of making slow all the users that
got this right.
Uninit was stored to memory at:
eth_header_parse+0xb8/0x110 net/ethernet/eth.c:204
dev_parse_header include/linux/netdevice.h:3158 [inline]
packet_rcv+0xefc/0x2050 net/packet/af_packet.c:2253
dev_queue_xmit_nit+0x114b/0x12a0 net/core/dev.c:2347
xmit_one net/core/dev.c:3584 [inline]
dev_hard_start_xmit+0x17d/0xa20 net/core/dev.c:3604
__dev_queue_xmit+0x3576/0x55e0 net/core/dev.c:4424
dev_queue_xmit include/linux/netdevice.h:3094 [inline]
Sanity check [1] in __bpf_redirect_common() does not really help, if
skb->len == 1 :/
/* Verify that a link layer header is carried */
if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
kfree_skb(skb);
return -ERANGE;
}
These bugs keep showing up.
[1]
commit 114039b342014680911c35bd6b72624180fd669a
Author: Stanislav Fomichev <sdf@fomichev.me>
Date: Mon Nov 21 10:03:39 2022 -0800
bpf: Move skb->len == 0 checks into __bpf_redirect
To avoid potentially breaking existing users.
Both mac/no-mac cases have to be amended; mac_header >= network_header
is not enough (verified with a new test, see next patch).
Fixes: fd1894224407 ("bpf: Don't redirect packets with invalid pkt_len")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
Link: https://lore.kernel.org/r/20221121180340.1983627-1-sdf@google.com
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
I sent an earlier patch, this went nowhere I am afraid.
https://www.spinics.net/lists/netdev/msg982652.html
Daniel, can you take a look and fix this in net/core/filter.c ?
On 10/2/24 9:27 AM, Eric Dumazet wrote:
> On Wed, Oct 2, 2024 at 7:39 AM Daniel Yang <danielyangkang@gmail.com> wrote:
>> pskb_expand_head doesn't initialize extra nhead bytes in header and
>> tail bytes, leading to KMSAN infoleak error. Fix by initializing data to
>> 0 with memset.
>>
>> Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
>> Tested-by: Daniel Yang <danielyangkang@gmail.com>
>> Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
> No no no.
>
> Please fix the root cause, instead of making slow all the users that
> got this right.
>
> Uninit was stored to memory at:
> eth_header_parse+0xb8/0x110 net/ethernet/eth.c:204
> dev_parse_header include/linux/netdevice.h:3158 [inline]
> packet_rcv+0xefc/0x2050 net/packet/af_packet.c:2253
> dev_queue_xmit_nit+0x114b/0x12a0 net/core/dev.c:2347
> xmit_one net/core/dev.c:3584 [inline]
> dev_hard_start_xmit+0x17d/0xa20 net/core/dev.c:3604
> __dev_queue_xmit+0x3576/0x55e0 net/core/dev.c:4424
> dev_queue_xmit include/linux/netdevice.h:3094 [inline]
>
>
> Sanity check [1] in __bpf_redirect_common() does not really help, if
> skb->len == 1 :/
>
> /* Verify that a link layer header is carried */
> if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
> kfree_skb(skb);
> return -ERANGE;
> }
>
> These bugs keep showing up.
>
> [1]
>
> commit 114039b342014680911c35bd6b72624180fd669a
> Author: Stanislav Fomichev <sdf@fomichev.me>
> Date: Mon Nov 21 10:03:39 2022 -0800
>
> bpf: Move skb->len == 0 checks into __bpf_redirect
>
> To avoid potentially breaking existing users.
>
> Both mac/no-mac cases have to be amended; mac_header >= network_header
> is not enough (verified with a new test, see next patch).
>
> Fixes: fd1894224407 ("bpf: Don't redirect packets with invalid pkt_len")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> Link: https://lore.kernel.org/r/20221121180340.1983627-1-sdf@google.com
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>
> I sent an earlier patch, this went nowhere I am afraid.
>
> https://www.spinics.net/lists/netdev/msg982652.html
>
> Daniel, can you take a look and fix this in net/core/filter.c ?
Ack, I'll have it on my todo list. Thanks!
I took a look at https://www.spinics.net/lists/netdev/msg982652.html and am a little confused since the patch adds a check instead of initializing the memory segment. Is the general assumption that any packet with uninitialized memory is ill formed and we need to drop? Also is there any documentation for internal macros/function calls for BPF because I was trying to look and couldn't find any.
On Thu, Oct 3, 2024 at 6:42 AM Daniel Yang <danielyangkang@gmail.com> wrote: > > I took a look at https://www.spinics.net/lists/netdev/msg982652.html > and am a little confused since the patch adds a check instead of > initializing the memory segment. > Is the general assumption that any packet with uninitialized memory is > ill formed and we need to drop? Also is there any documentation for > internal macros/function calls for BPF because I was trying to look > and couldn't find any. Callers wanting allocated memory to be cleared use __GFP_ZERO If we were forcing __GFP_ZERO all the time, network performance would be reduced by 30% at least. You are working around the real bug, just to silence a useful warning. As I explained earlier, the real bug is that some layers think the ethernet header (14 bytes) is present in the packet. Providing 14 zero bytes (instead of random bytes) would still be a bug. The real fix is to drop malicious packets when they are too small, like a NIC.
On Thu, Oct 3, 2024 at 12:56 AM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Oct 3, 2024 at 6:42 AM Daniel Yang <danielyangkang@gmail.com> wrote: > > > > I took a look at https://www.spinics.net/lists/netdev/msg982652.html > > and am a little confused since the patch adds a check instead of > > initializing the memory segment. > > Is the general assumption that any packet with uninitialized memory is > > ill formed and we need to drop? Also is there any documentation for > > internal macros/function calls for BPF because I was trying to look > > and couldn't find any. > > Callers wanting allocated memory to be cleared use __GFP_ZERO > If we were forcing __GFP_ZERO all the time, network performance would > be reduced by 30% at least. > > You are working around the real bug, just to silence a useful warning. > > As I explained earlier, the real bug is that some layers think the > ethernet header (14 bytes) is present in the packet. > > Providing 14 zero bytes (instead of random bytes) would still be a bug. > > The real fix is to drop malicious packets when they are too small, like a NIC. Interesting. Thank you for the clarification.
© 2016 - 2024 Red Hat, Inc.