Commit fa69ee5a authored by Marco Elver's avatar Marco Elver Committed by Jakub Kicinski
Browse files

net: switch to storing KCOV handle directly in sk_buff 

It turns out that usage of skb extensions can cause memory leaks. Ido
Schimmel reported: "[...] there are instances that blindly overwrite
'skb->extensions' by invoking skb_copy_header() after __alloc_skb()."

Therefore, give up on using skb extensions for KCOV handle, and instead
directly store kcov_handle in sk_buff.

Fixes: 6370cc3b ("net: add kcov handle to skb extensions")
Fixes: 85ce50d3 ("net: kcov: don't select SKB_EXTENSIONS when there is no NET")
Fixes: 97f53a08 ("net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling")
Link: https://lore.kernel.org/linux-wireless/20201121160941.GA485907@shredder.lan/


Reported-by: default avatarIdo Schimmel <idosch@idosch.org>
Signed-off-by: default avatarMarco Elver <elver@google.com>
Link: https://lore.kernel.org/r/20201125224840.2014773-1-elver@google.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent 0fca55ed

include/linux/skbuff.h

+13 −24
Original line number Diff line number Diff line
@@ -701,6 +701,7 @@ typedef unsigned char *sk_buff_data_t; 
 *	@transport_header: Transport layer header

 *	@network_header: Network layer header

 *	@mac_header: Link layer header

 *	@kcov_handle: KCOV remote handle for remote coverage collection

 *	@tail: Tail pointer

 *	@end: End pointer

 *	@head: Head of buffer
@@ -904,6 +905,10 @@ struct sk_buff { 
	__u16			network_header;

	__u16			mac_header;



#ifdef CONFIG_KCOV

	u64			kcov_handle;

#endif



	/* private: */

	__u32			headers_end[0];

	/* public: */
@@ -4150,9 +4155,6 @@ enum skb_ext_id { 
#endif

#if IS_ENABLED(CONFIG_MPTCP)

	SKB_EXT_MPTCP,

#endif

#if IS_ENABLED(CONFIG_KCOV)

	SKB_EXT_KCOV_HANDLE,

#endif

	SKB_EXT_NUM, /* must be last */

};
@@ -4608,35 +4610,22 @@ static inline void skb_reset_redirect(struct sk_buff *skb) 
#endif

}



#if IS_ENABLED(CONFIG_KCOV) && IS_ENABLED(CONFIG_SKB_EXTENSIONS)

static inline void skb_set_kcov_handle(struct sk_buff *skb,

				       const u64 kcov_handle)

{

	/* Do not allocate skb extensions only to set kcov_handle to zero

	 * (as it is zero by default). However, if the extensions are

	 * already allocated, update kcov_handle anyway since

	 * skb_set_kcov_handle can be called to zero a previously set

	 * value.

	 */

	if (skb_has_extensions(skb) || kcov_handle) {

		u64 *kcov_handle_ptr = skb_ext_add(skb, SKB_EXT_KCOV_HANDLE);



		if (kcov_handle_ptr)

			*kcov_handle_ptr = kcov_handle;

	}

#ifdef CONFIG_KCOV

	skb->kcov_handle = kcov_handle;

#endif

}



static inline u64 skb_get_kcov_handle(struct sk_buff *skb)

{

	u64 *kcov_handle = skb_ext_find(skb, SKB_EXT_KCOV_HANDLE);



	return kcov_handle ? *kcov_handle : 0;

}

#ifdef CONFIG_KCOV

	return skb->kcov_handle;

#else

static inline void skb_set_kcov_handle(struct sk_buff *skb,

				       const u64 kcov_handle) { }

static inline u64 skb_get_kcov_handle(struct sk_buff *skb) { return 0; }

#endif /* CONFIG_KCOV && CONFIG_SKB_EXTENSIONS */

	return 0;

#endif

}



#endif	/* __KERNEL__ */

#endif	/* _LINUX_SKBUFF_H */

lib/Kconfig.debug

+0 −1
Original line number Diff line number Diff line
@@ -1879,7 +1879,6 @@ config KCOV 
	depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS

	select DEBUG_FS

	select GCC_PLUGIN_SANCOV if !CC_HAS_SANCOV_TRACE_PC

	select SKB_EXTENSIONS if NET

	help

	  KCOV exposes kernel code coverage information in a form suitable

	  for coverage-guided fuzzing (randomized testing).

net/core/skbuff.c

+0 −6
Original line number Diff line number Diff line
@@ -4210,9 +4210,6 @@ static const u8 skb_ext_type_len[] = { 
#if IS_ENABLED(CONFIG_MPTCP)

	[SKB_EXT_MPTCP] = SKB_EXT_CHUNKSIZEOF(struct mptcp_ext),

#endif

#if IS_ENABLED(CONFIG_KCOV)

	[SKB_EXT_KCOV_HANDLE] = SKB_EXT_CHUNKSIZEOF(u64),

#endif

};



static __always_inline unsigned int skb_ext_total_length(void)
@@ -4229,9 +4226,6 @@ static __always_inline unsigned int skb_ext_total_length(void) 
#endif

#if IS_ENABLED(CONFIG_MPTCP)

		skb_ext_type_len[SKB_EXT_MPTCP] +

#endif

#if IS_ENABLED(CONFIG_KCOV)

		skb_ext_type_len[SKB_EXT_KCOV_HANDLE] +

#endif

		0;

}