[PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Kees Cook]

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

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Alexander Lobakin]

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

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Gustavo A. R. Silva]

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

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Alexander Lobakin]

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

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Gustavo A. R. Silva]

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

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Alexander Lobakin]

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

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Gustavo A. R. Silva]

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

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Gustavo A. R. Silva]

On 17/12/24 13:10, Christopher Ferris wrote:
> I verified that this does fix the compilation problem on Android. Thanks
> for working on this.

Awesome! :)

Thanks for confirming.

-Gustavo

> 
> Christopher
> 
> On Tue, Dec 17, 2024 at 10:31 AM Gustavo A. R. Silva <gustavo@embeddedor.com>
> wrote:
> 
>>
>>
>> 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
>>
> 

Re: [PATCH] UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

[Kees Cook]

On Tue, Dec 17, 2024 at 11:10:41AM -0800, Christopher Ferris wrote:
> I verified that this does fix the compilation problem on Android. Thanks
> for working on this.

Thanks! Yeah, let's use Alexander's solution instead of my proposed patch.

> > [0]
> > https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3

-Kees

-- 
Kees Cook