From: Aleksandr Nogikh <[email protected]>
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 <[email protected]>
---
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
On Mon, 12 Oct 2020 at 10:12, Johannes Berg <[email protected]> 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.
On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote:
> On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <[email protected]> wrote:
> >
> > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <[email protected]> wrote:
> > >
> > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <[email protected]> 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?