Hello Jakub,
On Wed, Oct 30, 2024 at 05:31:52PM -0700, Jakub Kicinski wrote:
> On Wed, 23 Oct 2024 04:38:01 -0700 Breno Leitao wrote:
> > + no longer reference valid memory locations. This deliberate invalidation
> > + helps expose code paths where proper pointer updating is neglected after a
> > + reallocation event.
> > +
> > + By creating these controlled fault scenarios, the system can catch instances
> > + where stale pointers are used, potentially leading to memory corruption or
> > + system instability.
> > +
> > + To select the interface to act on, write the network name to the following file:
> > + `/sys/kernel/debug/fail_skb_realloc/devname`
> > + If this field is left empty (which is the default value), skb reallocation
> > + will be forced on all network interfaces.
>
> Should we mention here that KASAN or some such is needed to catch
> the bugs? Chances are the resulting UAF will not crash and go unnoticed
> without KASAN.
What about adding something like this in the fail_skb_realloc section in
the fault-injection.rst file:
The effectiveness of this fault detection is enhanced when KASAN is
enabled, as it helps identify invalid memory references and
use-after-free (UAF) issues.
> > --- /dev/null
> > +++ b/net/core/skb_fault_injection.c
> > @@ -0,0 +1,103 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <linux/fault-inject.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/skbuff.h>
>
> alphabetic sort, please?
I thought I should use the reverse xmas tree structure. I will re-order
them alphabetically.
> > +static void reset_settings(void)
> > +{
> > + skb_realloc.filtered = false;
> > + memzero_explicit(&skb_realloc.devname, IFNAMSIZ);
>
> why _explicit ?
I thought the extra barrier would be helpful, but, it might not. I will
change it to a regular memset() if you think it is better.
> > +static ssize_t devname_write(struct file *file, const char __user *buffer,
> > + size_t count, loff_t *ppos)
> > +{
> > + ssize_t ret;
> > +
> > + reset_settings();
> > + ret = simple_write_to_buffer(&skb_realloc.devname, IFNAMSIZ,
> > + ppos, buffer, count);
> > + if (ret < 0)
> > + return ret;
>
> the buffer needs to be null terminated, like:
>
> skb_realloc.devname[IFNAMSIZ - 1] = '\0';
>
> no?
Yes, but isn't it what the next line do, with strim()?
> > + strim(skb_realloc.devname);
> > +
> > + if (strnlen(skb_realloc.devname, IFNAMSIZ))
> > + skb_realloc.filtered = true;
> > +
> > + return count;
> > +}
Thanks for the review!
--breno
