[PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[Ally Heev]

Uninitialized pointers with `__free` attribute can cause undefined
behavior as the memory assigned randomly to the pointer is freed
automatically when the pointer goes out of scope.

It is better to initialize and assign pointers with `__free`
attribute in one statement to ensure proper scope-based cleanup.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
Signed-off-by: Ally Heev <allyheev@gmail.com>
---
Changes in v3:
- fixed style issues
- Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com

Changes in v2:
- fixed non-pointer initialization to NULL
- NOTE: drop v1
- Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
---
 drivers/net/ethernet/intel/ice/ice_flow.c       | 5 +++--
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
--- a/drivers/net/ethernet/intel/ice/ice_flow.c
+++ b/drivers/net/ethernet/intel/ice/ice_flow.c
@@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
 			 struct ice_parser_profile *prof, enum ice_block blk)
 {
 	u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
-	struct ice_flow_prof_params *params __free(kfree);
 	u8 fv_words = hw->blk[blk].es.fvw;
 	int status;
 	int i, idx;
 
-	params = kzalloc(sizeof(*params), GFP_KERNEL);
+	struct ice_flow_prof_params *params __free(kfree) =
+		kzalloc(sizeof(*params), GFP_KERNEL);
+
 	if (!params)
 		return -ENOMEM;
 
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
  */
 static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
 {
-	struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
 	struct idpf_vc_xn_params xn_params = {
 		.vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
 		.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
@@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
 	ssize_t reply_sz;
 	int err = 0;
 
-	rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
+	struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
+		kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
+
 	if (!rcvd_regions)
 		return -ENOMEM;
 

---
base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7

Best regards,
-- 
Ally Heev <allyheev@gmail.com>

Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[Alexander Lobakin]

From: Ally Heev <allyheev@gmail.com>
Date: Thu, 06 Nov 2025 17:25:48 +0530

> Uninitialized pointers with `__free` attribute can cause undefined
> behavior as the memory assigned randomly to the pointer is freed
> automatically when the pointer goes out of scope.
> 
> It is better to initialize and assign pointers with `__free`
> attribute in one statement to ensure proper scope-based cleanup.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> Signed-off-by: Ally Heev <allyheev@gmail.com>
> ---
> Changes in v3:
> - fixed style issues
> - Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com
> 
> Changes in v2:
> - fixed non-pointer initialization to NULL
> - NOTE: drop v1
> - Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
> ---
>  drivers/net/ethernet/intel/ice/ice_flow.c       | 5 +++--
>  drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
>  			 struct ice_parser_profile *prof, enum ice_block blk)
>  {
>  	u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> -	struct ice_flow_prof_params *params __free(kfree);
>  	u8 fv_words = hw->blk[blk].es.fvw;
>  	int status;
>  	int i, idx;
>  
> -	params = kzalloc(sizeof(*params), GFP_KERNEL);
> +	struct ice_flow_prof_params *params __free(kfree) =
> +		kzalloc(sizeof(*params), GFP_KERNEL);

Please don't do it that way. It's not C++ with RAII and
declare-where-you-use.
Just leave the variable declarations where they are, but initialize them
with `= NULL`.

Variable declarations must be in one block and sorted from the longest
to the shortest.

But most important, I'm not even sure how you could trigger an
"undefined behaviour" here. Both here and below the variable tagged with
`__free` is initialized right after the declaration block, before any
return. So how to trigger an UB here?

> +
>  	if (!params)
>  		return -ENOMEM;
>  
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
>   */
>  static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
>  {
> -	struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
>  	struct idpf_vc_xn_params xn_params = {
>  		.vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
>  		.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
> @@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
>  	ssize_t reply_sz;
>  	int err = 0;
>  
> -	rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> +	struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
> +		kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> +
>  	if (!rcvd_regions)
>  		return -ENOMEM;

Same here, @rcvd_regions is initialized before the very first return, no
idea how one can provoke an UB here.

>  
> 
> ---
> base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
> 
> Best regards,

Thanks,
Olek

Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[ally heev]

On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
[..]
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> >  			 struct ice_parser_profile *prof, enum ice_block blk)
> >  {
> >  	u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > -	struct ice_flow_prof_params *params __free(kfree);
> >  	u8 fv_words = hw->blk[blk].es.fvw;
> >  	int status;
> >  	int i, idx;
> >  
> > -	params = kzalloc(sizeof(*params), GFP_KERNEL);
> > +	struct ice_flow_prof_params *params __free(kfree) =
> > +		kzalloc(sizeof(*params), GFP_KERNEL);
> 
> Please don't do it that way. It's not C++ with RAII and
> declare-where-you-use.
> Just leave the variable declarations where they are, but initialize them
> with `= NULL`.
> 
> Variable declarations must be in one block and sorted from the longest
> to the shortest.
> 
> But most important, I'm not even sure how you could trigger an
> "undefined behaviour" here. Both here and below the variable tagged with
> `__free` is initialized right after the declaration block, before any
> return. So how to trigger an UB here?

It doesn't occur here. But, many maintainers/developers consider it a
bad practice because if the function returns before initialization or
use of `goto` can cause such behaviors.

Here though, the definitions are still at the top right? Maybe I could
just sort them

> 
> > +
> >  	if (!params)
> >  		return -ENOMEM;
> >  
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > @@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
> >   */
> >  static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> >  {
> > -	struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
> >  	struct idpf_vc_xn_params xn_params = {
> >  		.vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
> >  		.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
> > @@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> >  	ssize_t reply_sz;
> >  	int err = 0;
> >  
> > -	rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > +	struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
> > +		kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > +
> >  	if (!rcvd_regions)
> >  		return -ENOMEM;
> 
> Same here, @rcvd_regions is initialized before the very first return, no
> idea how one can provoke an UB here.
> 
> >  
> > 
> > ---
> > base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> > change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
> > 
> > Best regards,
> 
> Thanks,
> Olek

Regards,
Ally

Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[Przemek Kitszel]

On 11/6/25 17:05, ally heev wrote:
> On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
> [..]
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
>>>   			 struct ice_parser_profile *prof, enum ice_block blk)
>>>   {
>>>   	u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
>>> -	struct ice_flow_prof_params *params __free(kfree);
>>>   	u8 fv_words = hw->blk[blk].es.fvw;
>>>   	int status;
>>>   	int i, idx;
>>>   
>>> -	params = kzalloc(sizeof(*params), GFP_KERNEL);
>>> +	struct ice_flow_prof_params *params __free(kfree) =
>>> +		kzalloc(sizeof(*params), GFP_KERNEL);
>>
>> Please don't do it that way. It's not C++ with RAII and
>> declare-where-you-use.
>> Just leave the variable declarations where they are, but initialize them
>> with `= NULL`.

+1

>>
>> Variable declarations must be in one block and sorted from the longest
>> to the shortest.
>>
>> But most important, I'm not even sure how you could trigger an
>> "undefined behaviour" here. Both here and below the variable tagged with
>> `__free` is initialized right after the declaration block, before any
>> return. So how to trigger an UB here?
> 
> It doesn't occur here. But, many maintainers/developers consider it a
> bad practice because if the function returns before initialization or
> use of `goto` can cause such behaviors.

we were bitten by that already, scenario is as follow:
0. have a good code w/o UB and w/o redundant = NULL
1. add some early return, say:
	if (dest_vsi == fdir_vsi)
		return -EINVAL;
2. almost granted that person adding 1. will forget to add = NULL to all
declarations marked __free

> 
> Here though, the definitions are still at the top right? Maybe I could
> just sort them

we discourage putting any operations, including allocations, that may
fail into the declarations block

Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[ally heev]

On Fri, 2025-11-07 at 06:39 +0100, Przemek Kitszel wrote:
> On 11/6/25 17:05, ally heev wrote:
> > On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
> > [..]
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > > > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> > > >   			 struct ice_parser_profile *prof, enum ice_block blk)
> > > >   {
> > > >   	u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > > > -	struct ice_flow_prof_params *params __free(kfree);
> > > >   	u8 fv_words = hw->blk[blk].es.fvw;
> > > >   	int status;
> > > >   	int i, idx;
> > > >   
> > > > -	params = kzalloc(sizeof(*params), GFP_KERNEL);
> > > > +	struct ice_flow_prof_params *params __free(kfree) =
> > > > +		kzalloc(sizeof(*params), GFP_KERNEL);
> > > 
> > > Please don't do it that way. It's not C++ with RAII and
> > > declare-where-you-use.
> > > Just leave the variable declarations where they are, but initialize them
> > > with `= NULL`.
> 
> +1
> 
> > > 
> > > Variable declarations must be in one block and sorted from the longest
> > > to the shortest.
> > > 
> > > But most important, I'm not even sure how you could trigger an
> > > "undefined behaviour" here. Both here and below the variable tagged with
> > > `__free` is initialized right after the declaration block, before any
> > > return. So how to trigger an UB here?
> > 
> > It doesn't occur here. But, many maintainers/developers consider it a
> > bad practice because if the function returns before initialization or
> > use of `goto` can cause such behaviors.
> 
> we were bitten by that already, scenario is as follow:
> 0. have a good code w/o UB and w/o redundant = NULL
> 1. add some early return, say:
> 	if (dest_vsi == fdir_vsi)
> 		return -EINVAL;
> 2. almost granted that person adding 1. will forget to add = NULL to all
> declarations marked __free
> 
> > 
> > Here though, the definitions are still at the top right? Maybe I could
> > just sort them
> 
> we discourage putting any operations, including allocations, that may
> fail into the declarations block
> 

Makes sense. I will just initialize them with NULL then

Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[Simon Horman]

On Thu, Nov 06, 2025 at 03:07:26PM +0100, Alexander Lobakin wrote:
> From: Ally Heev <allyheev@gmail.com>
> Date: Thu, 06 Nov 2025 17:25:48 +0530
> 
> > Uninitialized pointers with `__free` attribute can cause undefined
> > behavior as the memory assigned randomly to the pointer is freed
> > automatically when the pointer goes out of scope.
> > 
> > It is better to initialize and assign pointers with `__free`
> > attribute in one statement to ensure proper scope-based cleanup.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> > Signed-off-by: Ally Heev <allyheev@gmail.com>
> > ---
> > Changes in v3:
> > - fixed style issues
> > - Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com
> > 
> > Changes in v2:
> > - fixed non-pointer initialization to NULL
> > - NOTE: drop v1
> > - Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
> > ---
> >  drivers/net/ethernet/intel/ice/ice_flow.c       | 5 +++--
> >  drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> >  			 struct ice_parser_profile *prof, enum ice_block blk)
> >  {
> >  	u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > -	struct ice_flow_prof_params *params __free(kfree);
> >  	u8 fv_words = hw->blk[blk].es.fvw;
> >  	int status;
> >  	int i, idx;
> >  
> > -	params = kzalloc(sizeof(*params), GFP_KERNEL);
> > +	struct ice_flow_prof_params *params __free(kfree) =
> > +		kzalloc(sizeof(*params), GFP_KERNEL);
> 
> Please don't do it that way. It's not C++ with RAII and
> declare-where-you-use.
> Just leave the variable declarations where they are, but initialize them
> with `= NULL`.
> 
> Variable declarations must be in one block and sorted from the longest
> to the shortest.
> 
> But most important, I'm not even sure how you could trigger an
> "undefined behaviour" here. Both here and below the variable tagged with
> `__free` is initialized right after the declaration block, before any
> return. So how to trigger an UB here?

FWIIW, I'd prefer if we sidestepped this discussion entirely
by not using __free [1] in this driver.

It seems to me that for both functions updated by this
patch that can easily be achieved using an idiomatic
goto label to free on error.

[1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

...

Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute

[ally heev]

On Sat, 2025-11-08 at 16:36 +0000, Simon Horman wrote:
[..]
> > Please don't do it that way. It's not C++ with RAII and
> > declare-where-you-use.
> > Just leave the variable declarations where they are, but initialize them
> > with `= NULL`.
> > 
> > Variable declarations must be in one block and sorted from the longest
> > to the shortest.
> > 
> > But most important, I'm not even sure how you could trigger an
> > "undefined behaviour" here. Both here and below the variable tagged with
> > `__free` is initialized right after the declaration block, before any
> > return. So how to trigger an UB here?
> 
> FWIIW, I'd prefer if we sidestepped this discussion entirely
> by not using __free [1] in this driver.
> 
> It seems to me that for both functions updated by this
> patch that can easily be achieved using an idiomatic
> goto label to free on error.
> 
> [1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
> 
> ...

Understood. I will come-up with a new patch series for removing these
two instances

Regards,
Ally