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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a7adb8a7-800e-6761-b791-e877ff79210a@intel.com>
Date:   Tue, 16 Nov 2021 20:18:04 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Stefan Assmann <sassmann@...hat.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Brelinski, Tony" <tony.brelinski@...el.com>
Subject: Re: [PATCH net 06/10] iavf: prevent accidental free of filter
 structure

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@...el.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@...el.com>
>> Tested-by: Tony Brelinski <tony.brelinski@...el.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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
>>
>