include/uapi/linux/pkt_cls.h | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)
This switches to using a manually constructed form of struct tagging
to avoid issues with C++ being unable to parse tagged structs within
anonymous unions, even under 'extern "C"':
../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous union may only have public non-static data members [-fpermissive]
To avoid having multiple struct member lists, use a define to declare
them.
Reported-by: cferris@google.com
Closes: https://lore.kernel.org/linux-hardening/Z1HZpe3WE5As8UAz@google.com/
Fixes: 216203bdc228 ("UAPI: net/sched: Use __struct_group() in flex struct tc_u32_sel")
Link: https://lore.kernel.org/r/202412120927.943DFEDD@keescook
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: netdev@vger.kernel.org
---
include/uapi/linux/pkt_cls.h | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 2c32080416b5..02aee6ed6bf0 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -245,20 +245,28 @@ struct tc_u32_key {
int offmask;
};
+#define tc_u32_sel_hdr_members \
+ unsigned char flags; \
+ unsigned char offshift; \
+ unsigned char nkeys; \
+ __be16 offmask; \
+ __u16 off; \
+ short offoff; \
+ short hoff; \
+ __be32 hmask
+
+struct tc_u32_sel_hdr {
+ tc_u32_sel_hdr_members;
+};
+
struct tc_u32_sel {
- /* New members MUST be added within the __struct_group() macro below. */
- __struct_group(tc_u32_sel_hdr, hdr, /* no attrs */,
- unsigned char flags;
- unsigned char offshift;
- unsigned char nkeys;
-
- __be16 offmask;
- __u16 off;
- short offoff;
-
- short hoff;
- __be32 hmask;
- );
+ /* Open-coded struct_group() to avoid C++ errors. */
+ union {
+ struct tc_u32_sel_hdr hdr;
+ struct {
+ tc_u32_sel_hdr_members;
+ };
+ };
struct tc_u32_key keys[];
};
--
2.34.1
From: Kees Cook <kees@kernel.org>
Date: Mon, 16 Dec 2024 18:59:55 -0800
> This switches to using a manually constructed form of struct tagging
> to avoid issues with C++ being unable to parse tagged structs within
> anonymous unions, even under 'extern "C"':
>
> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous union may only have public non-static data members [-fpermissive]
I worked around that like this in the past: [0]
As I'm not sure it would be fine to fix every such occurrence manually
by open-coding.
What do you think?
>
> To avoid having multiple struct member lists, use a define to declare
> them.
>
> Reported-by: cferris@google.com
> Closes: https://lore.kernel.org/linux-hardening/Z1HZpe3WE5As8UAz@google.com/
> Fixes: 216203bdc228 ("UAPI: net/sched: Use __struct_group() in flex struct tc_u32_sel")
> Link: https://lore.kernel.org/r/202412120927.943DFEDD@keescook
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Cc: netdev@vger.kernel.org
> ---
> include/uapi/linux/pkt_cls.h | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 2c32080416b5..02aee6ed6bf0 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -245,20 +245,28 @@ struct tc_u32_key {
> int offmask;
> };
>
> +#define tc_u32_sel_hdr_members \
> + unsigned char flags; \
> + unsigned char offshift; \
> + unsigned char nkeys; \
> + __be16 offmask; \
> + __u16 off; \
> + short offoff; \
> + short hoff; \
> + __be32 hmask
> +
> +struct tc_u32_sel_hdr {
> + tc_u32_sel_hdr_members;
> +};
> +
> struct tc_u32_sel {
> - /* New members MUST be added within the __struct_group() macro below. */
> - __struct_group(tc_u32_sel_hdr, hdr, /* no attrs */,
> - unsigned char flags;
> - unsigned char offshift;
> - unsigned char nkeys;
> -
> - __be16 offmask;
> - __u16 off;
> - short offoff;
> -
> - short hoff;
> - __be32 hmask;
> - );
> + /* Open-coded struct_group() to avoid C++ errors. */
> + union {
> + struct tc_u32_sel_hdr hdr;
> + struct {
> + tc_u32_sel_hdr_members;
> + };
> + };
> struct tc_u32_key keys[];
> };
[0]
https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3
Thanks,
Olek
On 17/12/24 08:55, Alexander Lobakin wrote:
> From: Kees Cook <kees@kernel.org>
> Date: Mon, 16 Dec 2024 18:59:55 -0800
>
>> This switches to using a manually constructed form of struct tagging
>> to avoid issues with C++ being unable to parse tagged structs within
>> anonymous unions, even under 'extern "C"':
>>
>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous union may only have public non-static data members [-fpermissive]
>
> I worked around that like this in the past: [0]
> As I'm not sure it would be fine to fix every such occurrence manually
> by open-coding.
> What do you think?
The thing is that, in this particular case, we need a struct tag to change
the type of an object in another struct. See:
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h
index 9050568a034c..64663112cad8 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h
@@ -242,7 +242,7 @@ struct cxgb4_next_header {
* field's value to jump to next header such as IHL field
* in IPv4 header.
*/
- struct tc_u32_sel sel;
+ struct tc_u32_sel_hdr sel;
struct tc_u32_key key;
/* location of jump to make */
const struct cxgb4_match_field *jump;;
You can also take a look at the original series:
https://lore.kernel.org/linux-hardening/cover.1723586870.git.gustavoars@kernel.org/
Thanks
--
Gustavo
From: Gustavo A. R. Silva <gustavo@embeddedor.com>
Date: Tue, 17 Dec 2024 09:58:28 -0600
>
>
> On 17/12/24 08:55, Alexander Lobakin wrote:
>> From: Kees Cook <kees@kernel.org>
>> Date: Mon, 16 Dec 2024 18:59:55 -0800
>>
>>> This switches to using a manually constructed form of struct tagging
>>> to avoid issues with C++ being unable to parse tagged structs within
>>> anonymous unions, even under 'extern "C"':
>>>
>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct
>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous
>>> union may only have public non-static data members [-fpermissive]
>>
>> I worked around that like this in the past: [0]
>> As I'm not sure it would be fine to fix every such occurrence manually
>> by open-coding.
>> What do you think?
>
> The thing is that, in this particular case, we need a struct tag to change
> the type of an object in another struct. See:
But the fix I mentioned still allows you to specify a tag in C code...
cxgb4 is for sure not C++.
>
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h b/
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h
> index 9050568a034c..64663112cad8 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h
> @@ -242,7 +242,7 @@ struct cxgb4_next_header {
> * field's value to jump to next header such as IHL field
> * in IPv4 header.
> */
> - struct tc_u32_sel sel;
> + struct tc_u32_sel_hdr sel;
> struct tc_u32_key key;
> /* location of jump to make */
> const struct cxgb4_match_field *jump;;
>
> You can also take a look at the original series:
>
> https://lore.kernel.org/linux-hardening/
> cover.1723586870.git.gustavoars@kernel.org/
>
> Thanks
> --
> Gustavo
Thanks,
Olek
On 17/12/24 10:04, Alexander Lobakin wrote: > From: Gustavo A. R. Silva <gustavo@embeddedor.com> > Date: Tue, 17 Dec 2024 09:58:28 -0600 > >> >> >> On 17/12/24 08:55, Alexander Lobakin wrote: >>> From: Kees Cook <kees@kernel.org> >>> Date: Mon, 16 Dec 2024 18:59:55 -0800 >>> >>>> This switches to using a manually constructed form of struct tagging >>>> to avoid issues with C++ being unable to parse tagged structs within >>>> anonymous unions, even under 'extern "C"': >>>> >>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>>> union may only have public non-static data members [-fpermissive] >>> >>> I worked around that like this in the past: [0] >>> As I'm not sure it would be fine to fix every such occurrence manually >>> by open-coding. >>> What do you think? >> >> The thing is that, in this particular case, we need a struct tag to change >> the type of an object in another struct. See: > > But the fix I mentioned still allows you to specify a tag in C code... > cxgb4 is for sure not C++. Oh yes, I see what you mean. If it works, then you should probably submit that patch upstream. :) Thanks -- Gustavo
From: Gustavo A. R. Silva <gustavo@embeddedor.com> Date: Tue, 17 Dec 2024 10:25:29 -0600 > > > On 17/12/24 10:04, Alexander Lobakin wrote: >> From: Gustavo A. R. Silva <gustavo@embeddedor.com> >> Date: Tue, 17 Dec 2024 09:58:28 -0600 >> >>> >>> >>> On 17/12/24 08:55, Alexander Lobakin wrote: >>>> From: Kees Cook <kees@kernel.org> >>>> Date: Mon, 16 Dec 2024 18:59:55 -0800 >>>> >>>>> This switches to using a manually constructed form of struct tagging >>>>> to avoid issues with C++ being unable to parse tagged structs within >>>>> anonymous unions, even under 'extern "C"': >>>>> >>>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>>>> union may only have public non-static data members [-fpermissive] >>>> >>>> I worked around that like this in the past: [0] >>>> As I'm not sure it would be fine to fix every such occurrence manually >>>> by open-coding. >>>> What do you think? >>> >>> The thing is that, in this particular case, we need a struct tag to >>> change >>> the type of an object in another struct. See: >> >> But the fix I mentioned still allows you to specify a tag in C code... >> cxgb4 is for sure not C++. > > > Oh yes, I see what you mean. If it works, then you should probably > submit that > patch upstream. :) I added it to my CI tree and will wait for a report (24-36 hrs) before sending. In the meantime, feel free to test whether it solves your issue and give a Tested-by (or an error report :)). BTW, I mentioned in the commit message back in 2022 that some C++ standards support tagged structs with anonymous unions (I don't remember that already). Would it make sense to use a separate #define not for the whole __cplusplus, but only for certain standards? > > Thanks > -- > Gustavo Thanks, Olek
On 17/12/24 10:54, Alexander Lobakin wrote: > From: Gustavo A. R. Silva <gustavo@embeddedor.com> > Date: Tue, 17 Dec 2024 10:25:29 -0600 > >> >> >> On 17/12/24 10:04, Alexander Lobakin wrote: >>> From: Gustavo A. R. Silva <gustavo@embeddedor.com> >>> Date: Tue, 17 Dec 2024 09:58:28 -0600 >>> >>>> >>>> >>>> On 17/12/24 08:55, Alexander Lobakin wrote: >>>>> From: Kees Cook <kees@kernel.org> >>>>> Date: Mon, 16 Dec 2024 18:59:55 -0800 >>>>> >>>>>> This switches to using a manually constructed form of struct tagging >>>>>> to avoid issues with C++ being unable to parse tagged structs within >>>>>> anonymous unions, even under 'extern "C"': >>>>>> >>>>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>>>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>>>>> union may only have public non-static data members [-fpermissive] >>>>> >>>>> I worked around that like this in the past: [0] >>>>> As I'm not sure it would be fine to fix every such occurrence manually >>>>> by open-coding. >>>>> What do you think? >>>> >>>> The thing is that, in this particular case, we need a struct tag to >>>> change >>>> the type of an object in another struct. See: >>> >>> But the fix I mentioned still allows you to specify a tag in C code... >>> cxgb4 is for sure not C++. >> >> >> Oh yes, I see what you mean. If it works, then you should probably >> submit that >> patch upstream. :) > > I added it to my CI tree and will wait for a report (24-36 hrs) before > sending. In the meantime, feel free to test whether it solves your issue > and give a Tested-by (or an error report :)). Hopefully, Christopher can confirm whether this[0] resolves the issue he's seeing. > > BTW, I mentioned in the commit message back in 2022 that some C++ > standards support tagged structs with anonymous unions (I don't remember > that already). Would it make sense to use a separate #define not for the > whole __cplusplus, but only for certain standards? I'd say entirely preventing C++ from seeing the tag is cleaner and safer for now. Thanks -Gustavo [0] https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3
© 2016 - 2025 Red Hat, Inc.