Re: [PATCH net 06/10] iavf: prevent accidental free of filter structure

["Keller, Jacob E" <jacob.e.keller@intel.com>]

On 11/15/2021 11:24 PM, Stefan Assmann wrote:
> On 2021-11-15 15:59, Tony Nguyen wrote:
>> From: Jacob Keller <jacob.e.keller@intel.com>
>>
>> In iavf_config_clsflower, the filter structure could be accidentally
>> released at the end, if iavf_parse_cls_flower or iavf_handle_tclass ever
>> return a non-zero but positive value.
>>
>> In this case, the function continues through to the end, and will call
>> kfree() on the filter structure even though it has been added to the
>> linked list.
>>
>> This can actually happen because iavf_parse_cls_flower will return
>> a positive IAVF_ERR_CONFIG value instead of the traditional negative
>> error codes.
> 
> Hi Jacob,
> 
> where exactly does this happen?
> Looking at iavf_parse_cls_flower() I see all returns of IAVF_ERR_CONFIG
> as "return IAVF_ERR_CONFIG;" while IAVF_ERR_CONFIG is defined as
>         IAVF_ERR_CONFIG                         = -4,
> 
> I'm not opposed to this change, just wondering what's going on.
> 
>   Stefan
> 

Heh.

I don't have memory of the full context for the original work. We've
been going through and trying to pull in fixes that we've done for our
out-of-tree driver and get everything upstream.

At first I thought this might be because of some history where these
values used to be positive in the out-of-tree history at some point...
But I think this wasn't true. It is possible that some other flow
accidentally sends a positive value, but I've long since lost memory of
if I had an example of that. You're correct that IAVF_ERR_CONFIG is (and
has been in both upstream and out-of-tree code since its inception)
negative.

I don't think this change is harmful, but I think you're right in
pointing out the description isn't really valid.

I'm happy to re-write this commit message for clarity.

I do think switching to "if (err)" is more idiomatic and the correct
thing to do.

Thanks,
Jake

>> Fix this by ensuring that the kfree() check and error checks are
>> similar. Use the more idiomatic "if (err)" to catch all non-zero error
>> codes.
>>
>> Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> Tested-by: Tony Brelinski <tony.brelinski@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>>  drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 76c4ca0f055e..9c68c8628512 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -3108,11 +3108,11 @@ static int iavf_configure_clsflower(struct iavf_adapter *adapter,
>>  	/* start out with flow type and eth type IPv4 to begin with */
>>  	filter->f.flow_type = VIRTCHNL_TCP_V4_FLOW;
>>  	err = iavf_parse_cls_flower(adapter, cls_flower, filter);
>> -	if (err < 0)
>> +	if (err)
>>  		goto err;
>>  
>>  	err = iavf_handle_tclass(adapter, tc, filter);
>> -	if (err < 0)
>> +	if (err)
>>  		goto err;
>>  
>>  	/* add filter to the list */
>> -- 
>> 2.31.1
>>
>