* [PATCH] net: randomize layout of struct net_device
@ 2025-06-02 13:59 Pranav Tyagi
2025-06-02 14:07 ` Kees Cook
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Pranav Tyagi @ 2025-06-02 13:59 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, horms, keescook,
netdev, linux-kernel, skhan, linux-kernel-mentees
Cc: Pranav Tyagi
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
Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
---
include/linux/netdevice.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7ea022750e4e..0caff664ef3a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2077,7 +2077,11 @@ enum netdev_reg_state {
* moves out.
*/
+#ifdef CONFIG_RANDSTRUCT
+struct __randomize_layout net_device {
+#else
struct net_device {
+#endif
/* Cacheline organization can be found documented in
* Documentation/networking/net_cachelines/net_device.rst.
* Please update the document when adding new fields.
--
2.49.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 13:59 [PATCH] net: randomize layout of struct net_device Pranav Tyagi
@ 2025-06-02 14:07 ` Kees Cook
2025-06-02 14:22 ` Pranav Tyagi
2025-06-02 14:07 ` Greg KH
2025-06-02 14:46 ` Andrew Lunn
2 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2025-06-02 14:07 UTC (permalink / raw)
To: Pranav Tyagi, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
keescook, netdev, linux-kernel, skhan, linux-kernel-mentees
On June 2, 2025 6:59:32 AM PDT, Pranav Tyagi <pranav.tyagi03@gmail.com> 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
>Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
>---
> include/linux/netdevice.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index 7ea022750e4e..0caff664ef3a 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -2077,7 +2077,11 @@ enum netdev_reg_state {
> * moves out.
> */
>
>+#ifdef CONFIG_RANDSTRUCT
>+struct __randomize_layout net_device {
>+#else
> struct net_device {
>+#endif
There no need for the ifdef. Also these traditionally go at the end, between } and ;. See other examples in the tree.
-Kees
> /* Cacheline organization can be found documented in
> * Documentation/networking/net_cachelines/net_device.rst.
> * Please update the document when adding new fields.
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 13:59 [PATCH] net: randomize layout of struct net_device Pranav Tyagi
2025-06-02 14:07 ` Kees Cook
@ 2025-06-02 14:07 ` Greg KH
2025-06-06 15:04 ` Pranav Tyagi
2025-06-02 14:46 ` Andrew Lunn
2 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2025-06-02 14:07 UTC (permalink / raw)
To: Pranav Tyagi
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, keescook,
netdev, linux-kernel, skhan, linux-kernel-mentees
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
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> ---
> include/linux/netdevice.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7ea022750e4e..0caff664ef3a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2077,7 +2077,11 @@ enum netdev_reg_state {
> * moves out.
> */
>
> +#ifdef CONFIG_RANDSTRUCT
> +struct __randomize_layout net_device {
> +#else
> struct net_device {
> +#endif
Are you sure the #ifdef is needed?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 14:07 ` Kees Cook
@ 2025-06-02 14:22 ` Pranav Tyagi
2025-06-02 14:43 ` Pranav Tyagi
0 siblings, 1 reply; 12+ messages in thread
From: Pranav Tyagi @ 2025-06-02 14:22 UTC (permalink / raw)
To: Kees Cook
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, keescook,
netdev, linux-kernel, skhan, linux-kernel-mentees
On Mon, Jun 2, 2025 at 7:37 PM Kees Cook <kees@kernel.org> wrote:
>
>
>
> On June 2, 2025 6:59:32 AM PDT, Pranav Tyagi <pranav.tyagi03@gmail.com> 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
> >Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> >---
> > include/linux/netdevice.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >index 7ea022750e4e..0caff664ef3a 100644
> >--- a/include/linux/netdevice.h
> >+++ b/include/linux/netdevice.h
> >@@ -2077,7 +2077,11 @@ enum netdev_reg_state {
> > * moves out.
> > */
> >
> >+#ifdef CONFIG_RANDSTRUCT
> >+struct __randomize_layout net_device {
> >+#else
> > struct net_device {
> >+#endif
>
> There no need for the ifdef. Also these traditionally go at the end, between } and ;. See other examples in the tree.
>
> -Kees
Thanks for the feedback. I will update the patch accordingly.
>
> > /* Cacheline organization can be found documented in
> > * Documentation/networking/net_cachelines/net_device.rst.
> > * Please update the document when adding new fields.
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 14:22 ` Pranav Tyagi
@ 2025-06-02 14:43 ` Pranav Tyagi
0 siblings, 0 replies; 12+ messages in thread
From: Pranav Tyagi @ 2025-06-02 14:43 UTC (permalink / raw)
To: Kees Cook
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, keescook,
netdev, linux-kernel, skhan, linux-kernel-mentees
On Mon, Jun 2, 2025 at 7:52 PM Pranav Tyagi <pranav.tyagi03@gmail.com> wrote:
>
> On Mon, Jun 2, 2025 at 7:37 PM Kees Cook <kees@kernel.org> wrote:
> >
> >
> >
> > On June 2, 2025 6:59:32 AM PDT, Pranav Tyagi <pranav.tyagi03@gmail.com> 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
> > >Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > >---
> > > include/linux/netdevice.h | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > >index 7ea022750e4e..0caff664ef3a 100644
> > >--- a/include/linux/netdevice.h
> > >+++ b/include/linux/netdevice.h
> > >@@ -2077,7 +2077,11 @@ enum netdev_reg_state {
> > > * moves out.
> > > */
> > >
> > >+#ifdef CONFIG_RANDSTRUCT
> > >+struct __randomize_layout net_device {
> > >+#else
> > > struct net_device {
> > >+#endif
> >
> > There no need for the ifdef. Also these traditionally go at the end, between } and ;. See other examples in the tree.
> >
> > -Kees
>
> Thanks for the feedback. I will update the patch accordingly.
I also wanted to know if the __randomize_layout can go in between
struct and net_device as ____cacheline_aligned
is placed between } and ; at the end of the struct?
> >
> > > /* Cacheline organization can be found documented in
> > > * Documentation/networking/net_cachelines/net_device.rst.
> > > * Please update the document when adding new fields.
> >
> > --
> > Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 13:59 [PATCH] net: randomize layout of struct net_device Pranav Tyagi
2025-06-02 14:07 ` Kees Cook
2025-06-02 14:07 ` Greg KH
@ 2025-06-02 14:46 ` Andrew Lunn
2025-06-02 18:03 ` Kees Cook
2 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2025-06-02 14:46 UTC (permalink / raw)
To: Pranav Tyagi
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, keescook,
netdev, linux-kernel, skhan, linux-kernel-mentees
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
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> ---
> include/linux/netdevice.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7ea022750e4e..0caff664ef3a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2077,7 +2077,11 @@ enum netdev_reg_state {
> * moves out.
> */
>
> +#ifdef CONFIG_RANDSTRUCT
> +struct __randomize_layout net_device {
> +#else
> struct net_device {
> +#endif
> /* Cacheline organization can be found documented in
> * Documentation/networking/net_cachelines/net_device.rst.
> * Please update the document when adding new fields.
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.
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.
Is there documentation somewhere about what __randomize_layout
actually does? Given you are posting to a networking mailing list, you
should not assume the developers here are deep into how the compiler
works, and want to include a link to documentation, so we can see this
is actually safe to do.
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 14:46 ` Andrew Lunn
@ 2025-06-02 18:03 ` Kees Cook
2025-06-02 19:06 ` Andrew Lunn
0 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2025-06-02 18:03 UTC (permalink / raw)
To: Andrew Lunn
Cc: Pranav Tyagi, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
netdev, linux-kernel, skhan, linux-kernel-mentees
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?
> 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.
Anything that must stay ordered due to invisible assumptions would need
a distinct anonymous array to keep them together.
> Is there documentation somewhere about what __randomize_layout
> actually does? Given you are posting to a networking mailing list, you
> should not assume the developers here are deep into how the compiler
> works, and want to include a link to documentation, so we can see this
> is actually safe to do.
The basic details are in security/Kconfig.hardening in the "choice" following
the CC_HAS_RANDSTRUCT entry.
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 18:03 ` Kees Cook
@ 2025-06-02 19:06 ` Andrew Lunn
2025-06-06 14:55 ` Pranav Tyagi
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2025-06-02 19:06 UTC (permalink / raw)
To: Kees Cook
Cc: Pranav Tyagi, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
netdev, linux-kernel, skhan, linux-kernel-mentees
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 19:06 ` Andrew Lunn
@ 2025-06-06 14:55 ` Pranav Tyagi
2025-06-06 15:42 ` Eric Dumazet
0 siblings, 1 reply; 12+ messages in thread
From: Pranav Tyagi @ 2025-06-06 14:55 UTC (permalink / raw)
To: Andrew Lunn
Cc: Kees Cook, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
netdev, linux-kernel, skhan, linux-kernel-mentees
On Tue, Jun 3, 2025 at 12:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> 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
Resending to the list—my previous reply was accidentally sent off-list.
Apologies for the delayed response, and thank you
all for the detailed feedback.
Regarding the concern about breaking functionality,
I did compile and boot the kernel successfully with
this change, and everything appeared to work as
expected during basic testing. However, I agree
that this is not a substitute for thorough
benchmarking.
You're absolutely right that applying
__randomize_layout to net_device will shuffle
structure fields and likely incur a performance
penalty. As mentioned, this is a trade-off that
targets hardening over performance. It's worth
noting that CONFIG_RANDSTRUCT has two options:
RANDSTRUCT_FULL and RANDSTRUCT_PERFORMANCE, with
the latter aiming to minimize the impact by only
shuffling less performance-critical members.
I’d appreciate guidance on which specific
benchmarking tests would be most appropriate to
quantify the performance impact. Based on your
suggestions, I plan to run benchmarks on a small
SoC (ARM/MIPS/RISC-V) with 1G NICs. However, I
currently don’t have access to high-end server
hardware with 10G/40G+ NICs, so I’ll start with the
systems I have and clearly note the limitations in
the revised commit message.
I’ll also update the commit message to reflect the
security vs performance trade-offs, mention
RANDSTRUCT_PERFORMANCE, and add a reference to
net_cachelines/net_device.rst to document the
assumption of structure layout.
Thanks again for the thoughtful review—I’ll revise
the patch accordingly.
Regards
Pranav Tyagi
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-02 14:07 ` Greg KH
@ 2025-06-06 15:04 ` Pranav Tyagi
0 siblings, 0 replies; 12+ messages in thread
From: Pranav Tyagi @ 2025-06-06 15:04 UTC (permalink / raw)
To: Greg KH
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, keescook,
netdev, linux-kernel, skhan, linux-kernel-mentees
On Mon, Jun 2, 2025 at 8:50 PM Greg KH <gregkh@linuxfoundation.org> 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
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > ---
> > include/linux/netdevice.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 7ea022750e4e..0caff664ef3a 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -2077,7 +2077,11 @@ enum netdev_reg_state {
> > * moves out.
> > */
> >
> > +#ifdef CONFIG_RANDSTRUCT
> > +struct __randomize_layout net_device {
> > +#else
> > struct net_device {
> > +#endif
>
> Are you sure the #ifdef is needed?
>
> thanks,
>
> greg k-h
Hi Greg,
No, the #ifdef is not required since __randomize_layout is defined
as a no-op when CONFIG_RANDSTRUCT is disabled.
I rechecked the documentation to confirm this.
Thanks for pointing it out!
I will remove the #ifdef and update the patch before resending.
Regards
Pranav Tyagi
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-06 14:55 ` Pranav Tyagi
@ 2025-06-06 15:42 ` Eric Dumazet
2025-06-06 19:46 ` Kees Cook
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2025-06-06 15:42 UTC (permalink / raw)
To: Pranav Tyagi
Cc: Andrew Lunn, Kees Cook, andrew+netdev, davem, kuba, pabeni, horms,
netdev, linux-kernel, skhan, linux-kernel-mentees
On Fri, Jun 6, 2025 at 7:55 AM Pranav Tyagi <pranav.tyagi03@gmail.com> wrote:
>
> On Tue, Jun 3, 2025 at 12:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > 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
>
> Resending to the list—my previous reply was accidentally sent off-list.
>
> Apologies for the delayed response, and thank you
> all for the detailed feedback.
>
> Regarding the concern about breaking functionality,
> I did compile and boot the kernel successfully with
> this change, and everything appeared to work as
> expected during basic testing. However, I agree
> that this is not a substitute for thorough
> benchmarking.
>
> You're absolutely right that applying
> __randomize_layout to net_device will shuffle
> structure fields and likely incur a performance
> penalty. As mentioned, this is a trade-off that
> targets hardening over performance. It's worth
> noting that CONFIG_RANDSTRUCT has two options:
> RANDSTRUCT_FULL and RANDSTRUCT_PERFORMANCE, with
> the latter aiming to minimize the impact by only
> shuffling less performance-critical members.
>
> I’d appreciate guidance on which specific
> benchmarking tests would be most appropriate to
> quantify the performance impact. Based on your
> suggestions, I plan to run benchmarks on a small
> SoC (ARM/MIPS/RISC-V) with 1G NICs. However, I
> currently don’t have access to high-end server
> hardware with 10G/40G+ NICs, so I’ll start with the
> systems I have and clearly note the limitations in
> the revised commit message.
>
> I’ll also update the commit message to reflect the
> security vs performance trade-offs, mention
> RANDSTRUCT_PERFORMANCE, and add a reference to
> net_cachelines/net_device.rst to document the
> assumption of structure layout.
>
> Thanks again for the thoughtful review—I’ll revise
> the patch accordingly.
>
Do you have evidence of added security on this particular structure ?
What particular bug could have been avoided with __randomize_layout ?
Most distros use CONFIG_RANDSTRUCT_NONE=y, I do not think
__randomize_layout has a future.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: randomize layout of struct net_device
2025-06-06 15:42 ` Eric Dumazet
@ 2025-06-06 19:46 ` Kees Cook
0 siblings, 0 replies; 12+ messages in thread
From: Kees Cook @ 2025-06-06 19:46 UTC (permalink / raw)
To: Eric Dumazet, Pranav Tyagi
Cc: Andrew Lunn, andrew+netdev, davem, kuba, pabeni, horms, netdev,
linux-kernel, skhan, linux-kernel-mentees
On June 6, 2025 8:42:45 AM PDT, Eric Dumazet <edumazet@google.com> wrote:
>Most distros use CONFIG_RANDSTRUCT_NONE=y
That is true. But distros don't strictly define our code base. :)
> I do not think __randomize_layout has a future.
It will remain an actively supported feature -- many high security systems (that build their own kernels) use it, along with other features where they have no problem trading performance for security.
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-06-06 19:46 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-02 13:59 [PATCH] net: randomize layout of struct net_device Pranav Tyagi
2025-06-02 14:07 ` Kees Cook
2025-06-02 14:22 ` Pranav Tyagi
2025-06-02 14:43 ` Pranav Tyagi
2025-06-02 14:07 ` Greg KH
2025-06-06 15:04 ` Pranav Tyagi
2025-06-02 14:46 ` Andrew Lunn
2025-06-02 18:03 ` Kees Cook
2025-06-02 19:06 ` Andrew Lunn
2025-06-06 14:55 ` Pranav Tyagi
2025-06-06 15:42 ` Eric Dumazet
2025-06-06 19:46 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).