* [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 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-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 ` 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-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-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).