| Message ID | 20201026150851.528148-1-aleksandrnogikh@gmail.com |
|---|---|
| Headers | show |
| Series | net, mac80211, kernel: enable KCOV remote coverage collection for 802.11 frame handling | expand |
On Mon, Oct 26, 2020 at 11:11 AM Aleksandr Nogikh <aleksandrnogikh@gmail.com> 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 kcov remote handle to > skb extensions. Default initialization in __alloc_skb and > __build_skb_around 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 during normal background network > processes or were intentionally injected from the user space. > > Signed-off-by: Aleksandr Nogikh <nogikh@google.com> > -- > v2 -> v3: > * Reimplemented this change. Now kcov handle is added to skb > extensions instead of sk_buff. > v1 -> v2: > * Updated the commit message. > --- > include/linux/skbuff.h | 31 +++++++++++++++++++++++++++++++ > net/core/skbuff.c | 11 +++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index a828cf99c521..b63d90faa8e9 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -4150,6 +4150,9 @@ enum skb_ext_id { > #endif > #if IS_ENABLED(CONFIG_MPTCP) > SKB_EXT_MPTCP, > +#endif > +#if IS_ENABLED(CONFIG_KCOV) > + SKB_EXT_KCOV_HANDLE, > #endif > SKB_EXT_NUM, /* must be last */ > }; > @@ -4605,5 +4608,33 @@ static inline void skb_reset_redirect(struct sk_buff *skb) > #endif > } > > +#ifdef CONFIG_KCOV > + > +static inline void skb_set_kcov_handle(struct sk_buff *skb, const u64 kcov_handle) > +{ > + /* No reason to allocate skb extensions to set kcov_handle if kcov_handle is 0. */ If the handle does not need to be set if zero, why then set it if the skb has extensions? > + if (skb_has_extensions(skb) || kcov_handle) { > + u64 *kcov_handle_ptr = skb_ext_add(skb, SKB_EXT_KCOV_HANDLE); skb_ext_add and skb_ext_find are not defined unless CONFIG_SKB_EXTENSIONS. Perhaps CONFIG_KCOV should be made to select that? > + > + if (kcov_handle_ptr) > + *kcov_handle_ptr = kcov_handle; > + } > +} > + > +static inline u64 skb_get_kcov_handle(struct sk_buff *skb) > +{ > + u64 *kcov_handle = skb_ext_find(skb, SKB_EXT_KCOV_HANDLE); > + > + return kcov_handle ? *kcov_handle : 0; > +} > + > +#else > + > +static inline void skb_set_kcov_handle(struct sk_buff *skb, const u64 kcov_handle) { } > + > +static inline u64 skb_get_kcov_handle(struct sk_buff *skb) { return 0; } > + > +#endif /* CONFIG_KCOV */
On Mon, Oct 26, 2020 at 7:57 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: [...] > If the handle does not need to be set if zero, why then set it if the > skb has extensions? The point of that condition is to avoid unnecessary allocations of skb extension objects. At the same time, one would expect skb_get_kcov_handle to return the latest value that was set via skb_set_kcov_handle. So if a buffer already has a non-zero kcov_handle and skb_set_kcov_handle is called to set it to zero, it should be set to zero. > skb_ext_add and skb_ext_find are not defined unless CONFIG_SKB_EXTENSIONS. > > Perhaps CONFIG_KCOV should be made to select that? Yes, thank you for pointing it out. I’ll fix it in the next version.
On Tue, Oct 27, 2020 at 8:31 AM Aleksandr Nogikh <nogikh@google.com> wrote: > > On Mon, Oct 26, 2020 at 7:57 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > [...] > > If the handle does not need to be set if zero, why then set it if the > > skb has extensions? > > The point of that condition is to avoid unnecessary allocations of skb extension > objects. At the same time, one would expect skb_get_kcov_handle to return the > latest value that was set via skb_set_kcov_handle. So if a buffer already has a > non-zero kcov_handle and skb_set_kcov_handle is called to set it to zero, it > should be set to zero. I see. I thought it was some best effort approach: if there happens to be space, use it, but don't allocate. Which I did not understand. Could you rephrase the comment to make more clear that this is about clearing a possibly previously set value. > > skb_ext_add and skb_ext_find are not defined unless CONFIG_SKB_EXTENSIONS. > > > > Perhaps CONFIG_KCOV should be made to select that? > > Yes, thank you for pointing it out. I’ll fix it in the next version.
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 as an skb extension. 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() v3: * 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. * 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: add kcov handle to skb extensions mac80211: add KCOV remote annotations to incoming frame processing include/linux/skbuff.h | 31 +++++++++++++++++++++++++++++++ include/net/mac80211.h | 2 ++ kernel/kcov.c | 2 ++ net/core/skbuff.c | 11 +++++++++++ net/mac80211/iface.c | 2 ++ 5 files changed, 48 insertions(+) base-commit: 2ef991b5fdbe828dc8fb8af473dab160729570ed