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

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

[Aleksandr Nogikh]

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 commits.
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()

This patch series conflicts with another proposed patch
http://lkml.kernel.org/r/223901affc7bd759b2d6995c2dbfbdd0a29bc88a.1602248029.git.andreyknvl@google.com
One of these patches needs to be rebased once the other one is merged.

v2:
* Moved KCOV annotations from ieee80211_tasklet_handler to
  ieee80211_rx.
* Updated kcov_common_handle() to return 0 if it is called in
  interrupt context.
* Updated the cover letter.
 
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 +++++++++++++++++++++
 include/net/mac80211.h |  2 ++
 kernel/kcov.c          |  2 ++
 net/core/skbuff.c      |  1 +
 net/mac80211/iface.c   |  2 ++
 5 files changed, 28 insertions(+)


base-commit: a804ab086e9de200e2e70600996db7fc14c91959
-- 
2.28.0.1011.ga647a8990f-goog

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

[Aleksandr Nogikh]

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 a hard 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. It will make the returned
value more reliable and also will make it possible to use
kcov_remote_handle in routines that can be called from any context.

Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
---
v2:
* Added this patch to the series.
---
 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.28.0.1011.ga647a8990f-goog

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

[Andrey Konovalov]

On Fri, Oct 9, 2020 at 7:02 PM Aleksandr Nogikh <a.nogikh@gmail.com> wrote:
>
> 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 a hard IRQ context, it
> will return a handle for the interrupted process. This may lead to
> unreliable and incorrect coverage collection.

FWIW it's the same for soft IRQ context.

>
> 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. It will make the returned
> value more reliable and also will make it possible to use
> kcov_remote_handle in routines that can be called from any context.
>
> Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
> ---
> v2:
> * Added this patch to the series.
> ---
>  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.28.0.1011.ga647a8990f-goog
>

Reviewed-by: Andrey Konovalov <andreyknvl@google.com>

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

[Aleksandr Nogikh]

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>
---
v2:
* Updated the commit message.
---
 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 f67631faa9aa..e7acd7d45b03 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.28.0.1011.ga647a8990f-goog

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

[Aleksandr Nogikh]

From: Aleksandr Nogikh <nogikh@google.com>

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

Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
---
v2:
* The commit now affects ieee80211_rx instead of
  ieee80211_tasklet_handler.
---
 include/net/mac80211.h | 2 ++
 net/mac80211/iface.c   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 4747d446179a..011d9e115ebb 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4496,7 +4496,9 @@ void ieee80211_rx_napi(struct ieee80211_hw *hw, struct ieee80211_sta *sta,
  */
 static inline void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb)
 {
+	kcov_remote_start_common(skb_get_kcov_handle(skb));
 	ieee80211_rx_napi(hw, NULL, skb, NULL);
+	kcov_remote_stop();
 }
 
 /**
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 240862a74a0f..482d2ae46e71 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1377,6 +1377,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;
@@ -1486,6 +1487,7 @@ static void ieee80211_iface_work(struct work_struct *work)
 		}
 
 		kfree_skb(skb);
+		kcov_remote_stop();
 	}
 
 	/* then other type-dependent work */
-- 
2.28.0.1011.ga647a8990f-goog

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

[Johannes Berg]

On 9 October 2020 19:01:59 CEST, Aleksandr Nogikh <a.nogikh@gmail.com> wrote:

>This patch series conflicts with another proposed patch
>http://lkml.kernel.org/r/223901affc7bd759b2d6995c2dbfbdd0a29bc88a.1602248029.git.andreyknvl@google.com
>One of these patches needs to be rebased once the other one is merged.


Maybe that other patch shouldn't do things that way though, and add new API (which the existing one could call with some kind of "all contexts" argument) instead, so it's only necessary to specify the context (mask?) where its actually needed (the few places in usb or e whatever)? 

Surely that would also look less tedious in the mac80211 code, for example.

And if you ever fix the nesting issue you'd have fewer places to modify again.

johannes
-- 
Sent from my phone. 

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

[Andrey Konovalov]

On Fri, Oct 9, 2020 at 7:13 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
>
>
> On 9 October 2020 19:01:59 CEST, Aleksandr Nogikh <a.nogikh@gmail.com> wrote:
>
> >This patch series conflicts with another proposed patch
> >http://lkml.kernel.org/r/223901affc7bd759b2d6995c2dbfbdd0a29bc88a.1602248029.git.andreyknvl@google.com
> >One of these patches needs to be rebased once the other one is merged.
>
>
> Maybe that other patch shouldn't do things that way though, and add new API (which the existing one could call with some kind of "all contexts" argument) instead, so it's only necessary to specify the context (mask?) where its actually needed (the few places in usb or e whatever)?
>
> Surely that would also look less tedious in the mac80211 code, for example.
>
> And if you ever fix the nesting issue you'd have fewer places to modify again.

Hi Johannes,

I initially hesitated to do that, as it would multiply the number of
kcov callbacks. But perhaps you're right and a clean API look
outweighs the rest. I will do this in v3.

Thanks!

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

[Johannes Berg]

On 11 October 2020 12:37:29 CEST, Andrey Konovalov <andreyknvl@google.com> wrote:
>I initially hesitated to do that, as it would multiply the number of
>kcov callbacks. But perhaps you're right and a clean API look
>outweighs the rest. I will do this in v3.


Yeah, OK, dunno. You can always make it an inline calling the "full" API so after compiling it's equivalent. But if course that still has the two APIs. It just seemed to the common case wouldn't worry really (have to) about these things, especially if you plan on changing it again later. 

johannes
-- 
Sent from my phone. 

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

[Johannes Berg]

On Fri, 2020-10-09 at 17:01 +0000, Aleksandr Nogikh wrote:
> 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.

Btw, it occurred to me that I don't know at all - is this related to
syzkaller? Or is there some other fuzzing you're working on? Can we get
the bug reports from it if it's different? :)


Also, unrelated to that (but I see Dmitry CC'ed), I started wondering if
it'd be helpful to have an easier raw 802.11 inject path on top of say
hwsim0; I noticed some syzbot reports where it created raw sockets, but
that only gets you into the *data* plane of the wifi stack, not into the
*management* plane. Theoretically you could add a monitor interface, but
right now the wifi setup (according to the current docs on github) is
using two IBSS interfaces.

Perhaps an inject path on the mac80211-hwsim "hwsim0" interface would be
something to consider? Or simply adding a third radio that's in
"monitor" mode, so that a raw socket bound to *that* interface can
inject with a radiotap header followed by an 802.11 frame, getting to
arbitrary frame handling code, not just data frames.

Any thoughts?

johannes

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

[Andrey Konovalov]

On Sun, Oct 11, 2020 at 8:50 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2020-10-09 at 17:01 +0000, Aleksandr Nogikh wrote:
> > 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.
>
> Btw, it occurred to me that I don't know at all - is this related to
> syzkaller? Or is there some other fuzzing you're working on? Can we get
> the bug reports from it if it's different? :)

Yes, all this is for syzkaller :)

>
> Also, unrelated to that (but I see Dmitry CC'ed), I started wondering if
> it'd be helpful to have an easier raw 802.11 inject path on top of say
> hwsim0; I noticed some syzbot reports where it created raw sockets, but
> that only gets you into the *data* plane of the wifi stack, not into the
> *management* plane. Theoretically you could add a monitor interface, but
> right now the wifi setup (according to the current docs on github) is
> using two IBSS interfaces.
>
> Perhaps an inject path on the mac80211-hwsim "hwsim0" interface would be
> something to consider? Or simply adding a third radio that's in
> "monitor" mode, so that a raw socket bound to *that* interface can
> inject with a radiotap header followed by an 802.11 frame, getting to
> arbitrary frame handling code, not just data frames.

I'll let Aleksandr address this part.

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

[Aleksandr Nogikh]

On Sun, 11 Oct 2020 at 21:50, Johannes Berg <johannes@sipsolutions.net> wrote:
[...]
> Also, unrelated to that (but I see Dmitry CC'ed), I started wondering if
> it'd be helpful to have an easier raw 802.11 inject path on top of say
> hwsim0; I noticed some syzbot reports where it created raw sockets, but
> that only gets you into the *data* plane of the wifi stack, not into the
> *management* plane. Theoretically you could add a monitor interface, but
> right now the wifi setup (according to the current docs on github) is
> using two IBSS interfaces.
>
> Perhaps an inject path on the mac80211-hwsim "hwsim0" interface would be
> something to consider? Or simply adding a third radio that's in
> "monitor" mode, so that a raw socket bound to *that* interface can
> inject with a radiotap header followed by an 802.11 frame, getting to
> arbitrary frame handling code, not just data frames.
>
> Any thoughts?
>
> johannes
>
*sending it again as I forgot to include Cc list*

Hi Johannes,

Thank you for sharing these ideas.

Currently we're injecting frames via mac80211_hwsim (by pretenting to
be wmediumd -
https://github.com/google/syzkaller/blob/4a77ae0bdc5cd75ebe88ce7c896aae6bbf457a29/executor/common_linux.h#L4922).
Injecting via RAW sockets would definitely be a much cleaner way, but
to do that we need to keep a separate monitor interface. That's pretty
hard as the fuzzer is constantly trying to break things, and direct
injection via mac80211_hwsim seems to be a much more robust way - it
will work as long as the virtual device is alive. hwsim0 is
unfortunately not available as fuzzer processes are run in separate
network namespaces, while this one is created during mac80211_hwsim
initialization.

The current approach seems to work fine for management frames - I was
able to create seed programs that inject valid management frames and
these frames have the expected effect on the subsystem (e.g. injecting
AP responses during scan/authentication/authorization forces a station
to believe that it has successfully connected to an AP).

--
Best Regards,
Aleksandr

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

[Johannes Berg]

On Mon, 2020-10-12 at 14:18 +0300, Aleksandr Nogikh wrote:
> 
> Currently we're injecting frames via mac80211_hwsim (by pretenting to
> be wmediumd -
> https://github.com/google/syzkaller/blob/4a77ae0bdc5cd75ebe88ce7c896aae6bbf457a29/executor/common_linux.h#L4922).

Ah, ok, of course that works too :-)

> Injecting via RAW sockets would definitely be a much cleaner way, but
> to do that we need to keep a separate monitor interface. That's pretty
> hard as the fuzzer is constantly trying to break things, and direct
> injection via mac80211_hwsim seems to be a much more robust way - it
> will work as long as the virtual device is alive. hwsim0 is
> unfortunately not available as fuzzer processes are run in separate
> network namespaces, while this one is created during mac80211_hwsim
> initialization.

Oh, OK. I guess we _could_ move that also to the new namespace or
something, but if the wmediumd approach works then I think it's not
worth it.

> The current approach seems to work fine for management frames - I was
> able to create seed programs that inject valid management frames and
> these frames have the expected effect on the subsystem (e.g. injecting
> AP responses during scan/authentication/authorization forces a station
> to believe that it has successfully connected to an AP).

Great!

johannes