From: Aleksandr Nogikh <[email protected]>
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
From: Aleksandr Nogikh <[email protected]>
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 <[email protected]>
---
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
On Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote:
> From: Aleksandr Nogikh <[email protected]>
>
> 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
On Wed, 7 Oct 2020 at 14:48, Johannes Berg <[email protected]> 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