[PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[Daniel Yang]

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.2

Re: [PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[Eric Dumazet]

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 ?

Re: [PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[Daniel Borkmann]

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!

Re: [PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[Daniel Yang]

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.

Re: [PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[Eric Dumazet]

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.

Re: [PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[kernel test robot]

Hi 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

Re: [PATCH] Fix KMSAN infoleak, initialize unused data in pskb_expand_head

[Daniel Yang]

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.