netdev - Re: [PATCH] net: randomize layout of struct net

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: <25d96fc0-c54b-4f24-a62b-cf68bf6da1a9@lunn.ch>
Date: Mon, 2 Jun 2025 21:06:49 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Kees Cook <kees@...nel.org>
Cc: Pranav Tyagi <pranav.tyagi03@...il.com>, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
	linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH] net: randomize layout of struct net_device

On Mon, Jun 02, 2025 at 11:03:18AM -0700, Kees Cook wrote:
> On Mon, Jun 02, 2025 at 04:46:14PM +0200, Andrew Lunn wrote:
> > On Mon, Jun 02, 2025 at 07:29:32PM +0530, Pranav Tyagi wrote:
> > > Add __randomize_layout to struct net_device to support structure layout
> > > randomization if CONFIG_RANDSTRUCT is enabled else the macro expands to
> > > do nothing. This enhances kernel protection by making it harder to
> > > predict the memory layout of this structure.
> > > 
> > > Link: https://github.com/KSPP/linux/issues/188
> 
> I would note that the TODO item in this Issue is "evaluate struct
> net_device".
> 
> > A dumb question i hope.
> > 
> > As you can see from this comment, some time and effort has been put
> > into the order of members in this structure so that those which are
> > accessed on the TX fast path are in the same cache line, and those on
> > the RX fast path are in the same cache line, and RX and TX fast paths
> > are in different cache lines, etc.
> 
> This is pretty well exactly one of the right questions to ask, and
> should be detailed in the commit message. Mainly: a) how do we know it
> will not break anything? b) why is net_device a struct that is likely
> to be targeted by an attacker?

For a), i doubt anything will break. The fact the structure has been
optimised for performance implies that members have been moved around,
and there are no comments in the structure saying don't move this,
otherwise bad things will happen.

There is a:

	u8			priv[] ____cacheline_aligned
				       __counted_by(priv_len);

at the end, but i assume RANDSTRUCT knows about them and won't move it.

As for b), i've no idea, not my area. There are a number of pointers
to structures contains ops. Maybe if you can take over those pointers,
point to something you can control, you can take control of the
Program Counter?

> > Does CONFIG_RANDSTRUCT understand this? It is safe to move members
> > around within a cache line. And it is safe to move whole cache lines
> > around. But it would be bad if the randomisation moved members between
> > cache lines, mixing up RX and TX fast path members, or spreading fast
> > path members over more cache lines, etc.
> 
> No, it'll move stuff all around. It's very much a security vs
> performance trade-off, but the systems being built with it are happy to
> take the hit.

It would be interesting to look back at the work optimising this
stricture to get a ball park figure how big a hit this is?

I also think some benchmark numbers would be interesting. I would
consider two different systems:

1) A small ARM/MIPS/RISC-V with 1G interfaces. The low amount of L1
cache on these systems mean that cache misses are important. So
spreading out the fast path members will be bad.

2) Desktop/Server class hardware, lots of cores, lots of cache, 10G,
40G or 100G interfaces. For these systems, i expect cache line
bouncing is more of an issue, so Rx and Tx fast path members want to
be kept in separate cache lines.

> The basic details are in security/Kconfig.hardening in the "choice" following
> the CC_HAS_RANDSTRUCT entry.

So i see two settings here. It looks like RANDSTRUCT_PERFORMANCE
should have minimal performance impact, so maybe this should be
mentioned in the commit message, and the benchmarks performed both on
full randomisation and with the performance setting.

I would also suggest a comment is added to the top of
Documentation/networking/net_cachelines/net_device.rst pointing out
this assumed RANDSTRUCT is disabled, and the existing comment in
struct net_device is also updated.

	Andrew