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

[PATCH 0/2] net, mac80211: 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 for the
mac80211 code that processes incoming 802.11 frames. These changes
make it possible to perform coverage-guided fuzzing in search of
remotely triggerable bugs.


The series consists of two commits.
1. Remember kcov_handle for each sk_buff. This can later be used to
enable remote coverage for other network subsystems.
2. Annotate the code that processes incoming 802.11 frames.

Aleksandr Nogikh (2):
  net: store KCOV remote handle in sk_buff
  mac80211: add KCOV remote annotations to incoming frame processing

 include/linux/skbuff.h | 21 +++++++++++++++++++++
 net/core/skbuff.c      |  1 +
 net/mac80211/iface.c   |  2 ++
 net/mac80211/main.c    |  2 ++
 4 files changed, 26 insertions(+)


base-commit: a804ab086e9de200e2e70600996db7fc14c91959
-- 
2.28.0.806.g8561365e88-goog

[PATCH 1/2] 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.

Enable coverage-guided frame injection by adding a kcov_handle
parameter to sk_buff structure. Initialization in __alloc_skb ensures
that no socket buffer that was generated during a system call will be
missed.

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>
---
 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.806.g8561365e88-goog

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Jakub Kicinski]

On Wed,  7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:
> 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.
> 
> Enable coverage-guided frame injection by adding a kcov_handle
> parameter to sk_buff structure. Initialization in __alloc_skb ensures
> that no socket buffer that was generated during a system call will be
> missed.
> 
> 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>

Could you use skb_extensions for this?

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Dmitry Vyukov]

On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed,  7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:
> > 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.
> >
> > Enable coverage-guided frame injection by adding a kcov_handle
> > parameter to sk_buff structure. Initialization in __alloc_skb ensures
> > that no socket buffer that was generated during a system call will be
> > missed.
> >
> > 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>
>
> Could you use skb_extensions for this?

Why? If for space, this is already under a non-production ifdef.

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Jakub Kicinski]

On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Wed,  7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:  
> > > 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.
> > >
> > > Enable coverage-guided frame injection by adding a kcov_handle
> > > parameter to sk_buff structure. Initialization in __alloc_skb ensures
> > > that no socket buffer that was generated during a system call will be
> > > missed.
> > >
> > > 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>  
> >
> > Could you use skb_extensions for this?  
> 
> Why? If for space, this is already under a non-production ifdef.

I understand, but the skb_ext infra is there for uncommon use cases
like this one. Any particular reason you don't want to use it? 
The slight LoC increase?

Is there any precedent for adding the kcov field to other performance
critical structures?

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Dmitry Vyukov]

On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Wed,  7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:
> > > > 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.
> > > >
> > > > Enable coverage-guided frame injection by adding a kcov_handle
> > > > parameter to sk_buff structure. Initialization in __alloc_skb ensures
> > > > that no socket buffer that was generated during a system call will be
> > > > missed.
> > > >
> > > > 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>
> > >
> > > Could you use skb_extensions for this?
> >
> > Why? If for space, this is already under a non-production ifdef.
>
> I understand, but the skb_ext infra is there for uncommon use cases
> like this one. Any particular reason you don't want to use it?
> The slight LoC increase?
>
> Is there any precedent for adding the kcov field to other performance
> critical structures?

I see. Yes, increase in complexity for no gain.
No, KCOV context wasn't added to anything as critical as sk_buff.
It seems there is no established practice both ways -- I don't see
anything debug-related in sk_buff nor in skb_ext_id...

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Aleksandr Nogikh]

On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
[...]
> > > > Could you use skb_extensions for this?
> > >
> > > Why? If for space, this is already under a non-production ifdef.
> >
> > I understand, but the skb_ext infra is there for uncommon use cases
> > like this one. Any particular reason you don't want to use it?
> > The slight LoC increase?
> >
> > Is there any precedent for adding the kcov field to other performance
> > critical structures?

It would be great to come to some conclusion on where exactly to store
kcov_handle. Technically, it is possible to use skb extensions for the
purpose, though it will indeed slightly increase the complexity.

Jakub, you think that kcov_handle should be added as an skb extension,
right?

Though I do not really object to moving the field, it still seems to
me that sk_buff itself is a better place. Right now skb extensions
store values that are local to specific protocols and that are only
meaningful in the context of these protocols (correct me if I'm
wrong). Although this patch only adds remote kcov coverage to the wifi
code, kcov_handle can be meaningful for other protocols as well - just
like the already existing sk_buff fields. So adding kcov_handle to skb
extensions will break this logical separation.

--
Best regards,
Aleksandr

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Jakub Kicinski]

On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote:
> On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > >
> > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:  
> > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:  
> [...]
> > > > > Could you use skb_extensions for this?  
> > > >
> > > > Why? If for space, this is already under a non-production ifdef.  
> > >
> > > I understand, but the skb_ext infra is there for uncommon use cases
> > > like this one. Any particular reason you don't want to use it?
> > > The slight LoC increase?
> > >
> > > Is there any precedent for adding the kcov field to other performance
> > > critical structures?  
> 
> It would be great to come to some conclusion on where exactly to store
> kcov_handle. Technically, it is possible to use skb extensions for the
> purpose, though it will indeed slightly increase the complexity.
> 
> Jakub, you think that kcov_handle should be added as an skb extension,
> right?

That'd be preferable. I understand with current use cases it doesn't
really matter, but history shows people come up with all sort of
wonderful use cases down the line. And when they do they rarely go back 
and fix such fundamental minutiae.

> Though I do not really object to moving the field, it still seems to
> me that sk_buff itself is a better place. Right now skb extensions
> store values that are local to specific protocols and that are only
> meaningful in the context of these protocols (correct me if I'm
> wrong). Although this patch only adds remote kcov coverage to the wifi
> code, kcov_handle can be meaningful for other protocols as well - just
> like the already existing sk_buff fields. So adding kcov_handle to skb
> extensions will break this logical separation.

It's not as much protocols as subsystems. The values are meaningful to
a subsystem which inserts them, that doesn't mean single layer of the
stack. If it was about storing layer's context we would just use
skb->cb.

So I think the kcov use matches pretty well. 

Florian, what's your take?

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Willem de Bruijn]

On Tue, Oct 13, 2020 at 12:50 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote:
> > On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote:
> > >
> > > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > >
> > > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> > > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > [...]
> > > > > > Could you use skb_extensions for this?
> > > > >
> > > > > Why? If for space, this is already under a non-production ifdef.
> > > >
> > > > I understand, but the skb_ext infra is there for uncommon use cases
> > > > like this one. Any particular reason you don't want to use it?
> > > > The slight LoC increase?
> > > >
> > > > Is there any precedent for adding the kcov field to other performance
> > > > critical structures?
> >
> > It would be great to come to some conclusion on where exactly to store
> > kcov_handle. Technically, it is possible to use skb extensions for the
> > purpose, though it will indeed slightly increase the complexity.
> >
> > Jakub, you think that kcov_handle should be added as an skb extension,
> > right?
>
> That'd be preferable. I understand with current use cases it doesn't
> really matter, but history shows people come up with all sort of
> wonderful use cases down the line. And when they do they rarely go back
> and fix such fundamental minutiae.
>
> > Though I do not really object to moving the field, it still seems to
> > me that sk_buff itself is a better place. Right now skb extensions
> > store values that are local to specific protocols and that are only
> > meaningful in the context of these protocols (correct me if I'm
> > wrong). Although this patch only adds remote kcov coverage to the wifi
> > code, kcov_handle can be meaningful for other protocols as well - just
> > like the already existing sk_buff fields. So adding kcov_handle to skb
> > extensions will break this logical separation.
>
> It's not as much protocols as subsystems. The values are meaningful to
> a subsystem which inserts them, that doesn't mean single layer of the
> stack. If it was about storing layer's context we would just use
> skb->cb.
>
> So I think the kcov use matches pretty well.

skb_extensions was the first thing that came to mind when I read this
patchset too. It is not specific to protocols.

We have long stopped growing sk_buff size.

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Aleksandr Nogikh]

On Thu, Oct 15, 2020 at 10:04 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Tue, Oct 13, 2020 at 12:50 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote:
> > > On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote:
> > > >
> > > > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > >
> > > > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> > > > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > [...]
> > > > > > > Could you use skb_extensions for this?
> > > > > >
> > > > > > Why? If for space, this is already under a non-production ifdef.
> > > > >
> > > > > I understand, but the skb_ext infra is there for uncommon use cases
> > > > > like this one. Any particular reason you don't want to use it?
> > > > > The slight LoC increase?
> > > > >
> > > > > Is there any precedent for adding the kcov field to other performance
> > > > > critical structures?
> > >
> > > It would be great to come to some conclusion on where exactly to store
> > > kcov_handle. Technically, it is possible to use skb extensions for the
> > > purpose, though it will indeed slightly increase the complexity.
> > >
> > > Jakub, you think that kcov_handle should be added as an skb extension,
> > > right?
> >
> > That'd be preferable. I understand with current use cases it doesn't
> > really matter, but history shows people come up with all sort of
> > wonderful use cases down the line. And when they do they rarely go back
> > and fix such fundamental minutiae.
> >
> > > Though I do not really object to moving the field, it still seems to
> > > me that sk_buff itself is a better place. Right now skb extensions
> > > store values that are local to specific protocols and that are only
> > > meaningful in the context of these protocols (correct me if I'm
> > > wrong). Although this patch only adds remote kcov coverage to the wifi
> > > code, kcov_handle can be meaningful for other protocols as well - just
> > > like the already existing sk_buff fields. So adding kcov_handle to skb
> > > extensions will break this logical separation.
> >
> > It's not as much protocols as subsystems. The values are meaningful to
> > a subsystem which inserts them, that doesn't mean single layer of the
> > stack. If it was about storing layer's context we would just use
> > skb->cb.
> >
> > So I think the kcov use matches pretty well.
>
> skb_extensions was the first thing that came to mind when I read this
> patchset too. It is not specific to protocols.
>
> We have long stopped growing sk_buff size.

Thank you all for your comments. I'll use skb extensions in v3 of the series.

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Johannes Berg]

On Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote:
> 
> @@ -904,6 +905,10 @@ struct sk_buff {
>  	__u16			network_header;
>  	__u16			mac_header;
>  
> +#ifdef CONFIG_KCOV
> +	u64			kcov_handle;
> +#endif
[...] 

> @@ -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());

Btw, you're only setting this here. It seems to me it would make sense
to copy it when the skb is copied, rather than then having it set to the
kcov handle of the (interrupted) task that was copying the skb?

johannes

Re: [PATCH 1/2] net: store KCOV remote handle in sk_buff

[Aleksandr Nogikh]

On Mon, 12 Oct 2020 at 10:12, Johannes Berg <johannes@sipsolutions.net> wrote:
[...]
> > @@ -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());
>
> Btw, you're only setting this here. It seems to me it would make sense
> to copy it when the skb is copied, rather than then having it set to the
> kcov handle of the (interrupted) task that was copying the skb?
>
> johannes
>

The field is added to the part of sk_buff that is between
headers_start and headers_end, therefore skb_copy will copy the field
to the newly created buffer. So in the case of copying it will be
initialized, and then overwritten. Probably, it's not the most
efficient way, but it seems to be the same for some other
fields that are initialized in __alloc_skb.

[PATCH 2/2] 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_tasklet_handler. This will enable coverage-guided fuzzing of
mac80211 code that processes incoming 802.11 frames.

Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
---
 net/mac80211/iface.c | 2 ++
 net/mac80211/main.c  | 2 ++
 2 files changed, 4 insertions(+)

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 */
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 523380aed92e..d7eebafc14e0 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -227,6 +227,7 @@ static void ieee80211_tasklet_handler(unsigned long data)
 
 	while ((skb = skb_dequeue(&local->skb_queue)) ||
 	       (skb = skb_dequeue(&local->skb_queue_unreliable))) {
+		kcov_remote_start_common(skb_get_kcov_handle(skb));
 		switch (skb->pkt_type) {
 		case IEEE80211_RX_MSG:
 			/* Clear skb->pkt_type in order to not confuse kernel
@@ -244,6 +245,7 @@ static void ieee80211_tasklet_handler(unsigned long data)
 			dev_kfree_skb(skb);
 			break;
 		}
+		kcov_remote_stop();
 	}
 }
 
-- 
2.28.0.806.g8561365e88-goog

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

[Johannes Berg]

On Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote:
> From: Aleksandr Nogikh <nogikh@google.com>
> 
> This patch series enables remote KCOV coverage collection for the
> mac80211 code that processes incoming 802.11 frames. These changes
> make it possible to perform coverage-guided fuzzing in search of
> remotely triggerable bugs.
> 
> 
> The series consists of two commits.
> 1. Remember kcov_handle for each sk_buff. This can later be used to
> enable remote coverage for other network subsystems.
> 2. Annotate the code that processes incoming 802.11 frames.
> 
> Aleksandr Nogikh (2):
>   net: store KCOV remote handle in sk_buff

Can you explain that a bit better? What is a "remote handle"? What does
it do in the SKB?

I guess I'd have to know more about "kcov_common_handle()" to understand
this bit.

>   mac80211: add KCOV remote annotations to incoming frame processing

This seems fine, but a bit too limited? You tagged
only ieee80211_tasklet_handler() which calls ieee80211_rx()
or ieee80211_tx_status(), but

1) I'm not even sure ieee80211_tx_status() counts (it's processing
locally generated frames after they round-tripped into the driver
(although in mesh it could be remote originated but retransmitted
frames, so I guess it makes some sense?); and

2) there are many other ways that ieee80211_rx() could get called.

It seems to me it'd make more sense to (also) annotate ieee80211_rx()
itself?

johannes

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

[Aleksandr Nogikh]

On Wed, 7 Oct 2020 at 14:48, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote:
[...]
> > Aleksandr Nogikh (2):
> >   net: store KCOV remote handle in sk_buff
>
> Can you explain that a bit better? What is a "remote handle"? What does
> it do in the SKB?
>
> I guess I'd have to know more about "kcov_common_handle()" to understand
> this bit.

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 some handling can happen
elsewhere (e.g. in separate kernel threads).

That is why remote KOV coverage collection was introduced. When it is
impossible to infer KCOV-related info just by looking at the currently
running process, we need to manually pass some information to the code
that is of interest to us.  The information takes the form of 64 bit
integers (remote handles). Zero is the special value that corresponds
to an empty handle. More details on KCOV and remote coverage
collection can be found here: Documentation/dev-tools/kcov.rst.

In this patch, we obtain the remote handle from KCOV (in this case by
executing kcov_common_handle()) and attach it to newly allocated
SKBs. If we're in 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). So when
kcov_remote_start_common(skb_get_kcov_handle(skb)) is executed, it is
possible to determine whether coverage is required and where it should
be stored.

I have just realized that the default kcov_handle initialization as it
was implemented in this patch is not really robust. If an skb is
allocated during a hard IRQ, kcov_common_handle() will return a remote
handle for the interrupted thread instead of returning 0, and that is
not desirable since it will occasionally lead to wrong kcov_handles. I
will fix it in the next version of the patch.

> >   mac80211: add KCOV remote annotations to incoming frame processing
>
> This seems fine, but a bit too limited? You tagged
> only ieee80211_tasklet_handler() which calls ieee80211_rx()
> or ieee80211_tx_status(), but
>
> 1) I'm not even sure ieee80211_tx_status() counts (it's processing
> locally generated frames after they round-tripped into the driver
> (although in mesh it could be remote originated but retransmitted
> frames, so I guess it makes some sense?); and
>
> 2) there are many other ways that ieee80211_rx() could get called.
>
> It seems to me it'd make more sense to (also) annotate ieee80211_rx()
> itself?

Yes, it definitely makes more sense to annotate ieee80211_rx()
directly. Collecting coverage for ieee80211_tx_status() does not seem
to be needed now and can be added later if there's a use case for it.

Thank you for the suggestion. I will implement it in the second
version of the patch.

--
Best regards,
Aleksandr