netdev - Re: [PATCH net-next v4] net: Implement fault injection forcing skb reallocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241031-hallowed-bizarre-curassow-ea16cc@leitao>
Date: Thu, 31 Oct 2024 02:41:18 -0700
From: Breno Leitao <leitao@...ian.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Jonathan Corbet <corbet@....net>, Akinobu Mita <akinobu.mita@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>, kernel-team@...a.com,
	Thomas Huth <thuth@...hat.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	"Borislav Petkov (AMD)" <bp@...en8.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	Xiongwei Song <xiongwei.song@...driver.com>,
	Mina Almasry <almasrymina@...gle.com>,
	Kuniyuki Iwashima <kuniyu@...zon.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	Oleksij Rempel <o.rempel@...gutronix.de>,
	"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v4] net: Implement fault injection forcing skb
 reallocation

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