[PATCH v6 0/3] net, mac80211, kernel: enable KCOV remote coverage collection for 802.11 frame handling

[PATCH v6 0/3] net, mac80211, kernel: enable KCOV remote coverage collection for 802.11 frame handling

[Marco Elver]

From: Aleksandr Nogikh <nogikh@google.com>

This patch series enables remote KCOV coverage collection during 802.11
frames processing. These changes make it possible to perform
coverage-guided fuzzing in search of remotely triggerable bugs.

Normally, KCOV collects coverage information for the code that is
executed inside the system call context. It is easy to identify where
that coverage should go and whether it should be collected at all by
looking at the current process. If KCOV was enabled on that process,
coverage will be stored in a buffer specific to that process.
Howerever, it is not always enough as handling can happen elsewhere
(e.g. in separate kernel threads).

When it is impossible to infer KCOV-related info just by looking at the
currently running process, one needs to manually pass some information
to the code that should be instrumented. The information takes the form
of 64 bit integers (KCOV remote handles). Zero is the special value that
corresponds to an empty handle. More details on KCOV and remote coverage
collection can be found in Documentation/dev-tools/kcov.rst.

The series consists of three patches:

1. Apply a minor fix to kcov_common_handle() so that it returns a valid
   handle (zero) when called in an interrupt context.

2. Take the remote handle from KCOV and attach it to newly allocated
   SKBs. If the allocation happens inside a system call context, the SKB
   will be tied to the process that issued the syscall (if that process
   is interested in remote coverage collection).

3. Annotate the code that processes incoming 802.11 frames with
   kcov_remote_start()/kcov_remote_stop().


v6:
* Revert usage of skb extensions due to potential memory leak. Patch 2/3 is now
  idential to that in v2.
* Patches 1/3 and 3/3 are otherwise identical to v5.

v5: https://lore.kernel.org/linux-wireless/20201029173620.2121359-1-aleksandrnogikh@gmail.com/
* Collecting remote coverate at ieee80211_rx_list() instead of
  ieee80211_rx()

v4: https://lkml.kernel.org/r/20201028182018.1780842-1-aleksandrnogikh@gmail.com
* CONFIG_SKB_EXTENSIONS is now automatically selected by CONFIG_KCOV.
* Elaborated on a minor optimization in skb_set_kcov_handle().

v3: https://lkml.kernel.org/r/20201026150851.528148-1-aleksandrnogikh@gmail.com
* kcov_handle is now stored in skb extensions instead of sk_buff
  itself.
* Updated the cover letter.

v2: https://lkml.kernel.org/r/20201009170202.103512-1-a.nogikh@gmail.com
* Moved KCOV annotations from ieee80211_tasklet_handler to
  ieee80211_rx.
* Updated kcov_common_handle() to return 0 if it is called in
  interrupt context.

v1: https://lkml.kernel.org/r/20201007101726.3149375-1-a.nogikh@gmail.com

Aleksandr Nogikh (3):
  kernel: make kcov_common_handle consider the current context
  net: store KCOV remote handle in sk_buff
  mac80211: add KCOV remote annotations to incoming frame processing

 include/linux/skbuff.h | 21 +++++++++++++++++++++
 kernel/kcov.c          |  2 ++
 net/core/skbuff.c      |  1 +
 net/mac80211/iface.c   |  2 ++
 net/mac80211/rx.c      | 16 +++++++++-------
 5 files changed, 35 insertions(+), 7 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v6 1/3] kernel: make kcov_common_handle consider the current context

[Marco Elver]

From: Aleksandr Nogikh <nogikh@google.com>

kcov_common_handle is a method that is used to obtain a "default" KCOV
remote handle of the current process. The handle can later be passed
to kcov_remote_start in order to collect coverage for the processing
that is initiated by one process, but done in another. For details see
Documentation/dev-tools/kcov.rst and comments in kernel/kcov.c.

Presently, if kcov_common_handle is called in an IRQ context, it will
return a handle for the interrupted process. This may lead to
unreliable and incorrect coverage collection.

Adjust the behavior of kcov_common_handle in the following way. If it
is called in a task context, return the common handle for the
currently running task. Otherwise, return 0.

Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
---
 kernel/kcov.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 6b8368be89c8..80bfe71bbe13 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -1023,6 +1023,8 @@ EXPORT_SYMBOL(kcov_remote_stop);
 /* See the comment before kcov_remote_start() for usage details. */
 u64 kcov_common_handle(void)
 {
+	if (!in_task())
+		return 0;
 	return current->kcov_handle;
 }
 EXPORT_SYMBOL(kcov_common_handle);
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v6 2/3] net: store KCOV remote handle in sk_buff

[Marco Elver]

From: Aleksandr Nogikh <nogikh@google.com>

Remote KCOV coverage collection enables coverage-guided fuzzing of the
code that is not reachable during normal system call execution. It is
especially helpful for fuzzing networking subsystems, where it is
common to perform packet handling in separate work queues even for the
packets that originated directly from the user space. More details can
be found in Documentation/dev-tools/kcov.rst.

Enable coverage-guided frame injection by adding a kcov_handle
parameter to sk_buff structure. Initializate this field in __alloc_skb
to kcov_common_handle() so that no socket buffer that was generated
during a system call is missed. For sk_buffs that were allocated in an
interrupt context, kcov_handle will be initialized to 0.

Code that is of interest and that performs packet processing should be
annotated with kcov_remote_start()/kcov_remote_stop().

An alternative approach is to determine kcov_handle solely on the
basis of the device/interface that received the specific socket
buffer. However, in this case it would be impossible to distinguish
between packets that originated from normal background network
processes and those that were intentionally injected from the user
space.

Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
v6:
* Revert usage of skb extensions in favour of directly storing the
  kcov_handle in in sk_buff.  skb extensions were leading to a memory
  leak as reported by Ido Schimmel:
  https://lore.kernel.org/linux-wireless/20201121160941.GA485907@shredder.lan/
---
 include/linux/skbuff.h | 21 +++++++++++++++++++++
 net/core/skbuff.c      |  1 +
 2 files changed, 22 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a828cf99c521..5639f27e05ef 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -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: */
@@ -4605,5 +4610,21 @@ static inline void skb_reset_redirect(struct sk_buff *skb)
 #endif
 }
 
+static inline void skb_set_kcov_handle(struct sk_buff *skb, const u64 kcov_handle)
+{
+#ifdef CONFIG_KCOV
+	skb->kcov_handle = kcov_handle;
+#endif
+}
+
+static inline u64 skb_get_kcov_handle(struct sk_buff *skb)
+{
+#ifdef CONFIG_KCOV
+	return skb->kcov_handle;
+#else
+	return 0;
+#endif
+}
+
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1ba8f0163744..2f27635c3e97 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -233,6 +233,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	skb->end = skb->tail + size;
 	skb->mac_header = (typeof(skb->mac_header))~0U;
 	skb->transport_header = (typeof(skb->transport_header))~0U;
+	skb_set_kcov_handle(skb, kcov_common_handle());
 
 	/* make sure we initialize shinfo sequentially */
 	shinfo = skb_shinfo(skb);
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v6 3/3] mac80211: add KCOV remote annotations to incoming frame processing

[Marco Elver]

From: Aleksandr Nogikh <nogikh@google.com>

Add KCOV remote annotations to ieee80211_iface_work() and
ieee80211_rx_list(). This will enable coverage-guided fuzzing of
mac80211 code that processes incoming 802.11 frames.

Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/iface.c |  2 ++
 net/mac80211/rx.c    | 16 +++++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 1be775979132..56a1bcea2c1c 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1356,6 +1356,7 @@ static void ieee80211_iface_work(struct work_struct *work)
 	while ((skb = skb_dequeue(&sdata->skb_queue))) {
 		struct ieee80211_mgmt *mgmt = (void *)skb->data;
 
+		kcov_remote_start_common(skb_get_kcov_handle(skb));
 		if (ieee80211_is_action(mgmt->frame_control) &&
 		    mgmt->u.action.category == WLAN_CATEGORY_BACK) {
 			int len = skb->len;
@@ -1465,6 +1466,7 @@ static void ieee80211_iface_work(struct work_struct *work)
 		}
 
 		kfree_skb(skb);
+		kcov_remote_stop();
 	}
 
 	/* then other type-dependent work */
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 1e2e5a406d58..09d1c9fb8872 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4742,6 +4742,8 @@ void ieee80211_rx_list(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta,
 
 	status->rx_flags = 0;
 
+	kcov_remote_start_common(skb_get_kcov_handle(skb));
+
 	/*
 	 * Frames with failed FCS/PLCP checksum are not returned,
 	 * all other frames are returned without radiotap header
@@ -4749,15 +4751,15 @@ void ieee80211_rx_list(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta,
 	 * Also, frames with less than 16 bytes are dropped.
 	 */
 	skb = ieee80211_rx_monitor(local, skb, rate);
-	if (!skb)
-		return;
-
-	ieee80211_tpt_led_trig_rx(local,
-			((struct ieee80211_hdr *)skb->data)->frame_control,
-			skb->len);
+	if (skb) {
+		ieee80211_tpt_led_trig_rx(local,
+					  ((struct ieee80211_hdr *)skb->data)->frame_control,
+					  skb->len);
 
-	__ieee80211_rx_handle_packet(hw, pubsta, skb, list);
+		__ieee80211_rx_handle_packet(hw, pubsta, skb, list);
+	}
 
+	kcov_remote_stop();
 	return;
  drop:
 	kfree_skb(skb);
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v6 0/3] net, mac80211, kernel: enable KCOV remote coverage collection for 802.11 frame handling

[Florian Westphal]

Marco Elver <elver@google.com> wrote:
[..]

> v6:
> * Revert usage of skb extensions due to potential memory leak. Patch 2/3 is now
>   idential to that in v2.
> * Patches 1/3 and 3/3 are otherwise identical to v5.

The earlier series was already applied to net-next, so you need to
rebase on top of net-next and include a revert of the patch that added
the kcov skb extension.

Also, please indicate the git tree that you want this applied to in the
subject.

Re: [PATCH v6 0/3] net, mac80211, kernel: enable KCOV remote coverage collection for 802.11 frame handling

[Jakub Kicinski]

On Wed, 25 Nov 2020 17:24:52 +0100 Marco Elver wrote:
> This patch series enables remote KCOV coverage collection during 802.11
> frames processing. These changes make it possible to perform
> coverage-guided fuzzing in search of remotely triggerable bugs.

Hi Marco, this stuff is only present in net-next, and were not reverted.

You need to rebase and replace the existing implementation.

Re: [PATCH v6 0/3] net, mac80211, kernel: enable KCOV remote coverage collection for 802.11 frame handling

[Marco Elver]

On Wed, 25 Nov 2020 at 17:45, Jakub Kicinski <kuba@kernel.org> wrote:
> On Wed, 25 Nov 2020 17:24:52 +0100 Marco Elver wrote:
> > This patch series enables remote KCOV coverage collection during 802.11
> > frames processing. These changes make it possible to perform
> > coverage-guided fuzzing in search of remotely triggerable bugs.
>
> Hi Marco, this stuff is only present in net-next, and were not reverted.
>
> You need to rebase and replace the existing implementation.

Ah, that's fine then. I wasn't sure if you can drop and replace things
in -net. I'll send a revert and change the implementation.