netdev - Re: [PATCH net-next 5/6] igb: Do not free q_vector unless new one was allocated

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 7 Nov 2022 10:35:14 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Leon Romanovsky <leon@...nel.org>,
        "Ruhl, Michael J" <michael.j.ruhl@...el.com>
CC:     "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "G, GurucharanX" <gurucharanx.g@...el.com>
Subject: Re: [PATCH net-next 5/6] igb: Do not free q_vector unless new one was
 allocated



On 11/7/2022 9:45 AM, Leon Romanovsky wrote:
> On Mon, Nov 07, 2022 at 01:55:58PM +0000, Ruhl, Michael J wrote:
>>> -----Original Message-----
>>> From: Leon Romanovsky <leon@...nel.org>
>>> Sent: Monday, November 7, 2022 2:03 AM
>>> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
>>> Cc: davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
>>> edumazet@...gle.com; Kees Cook <keescook@...omium.org>;
>>> netdev@...r.kernel.org; Brandeburg, Jesse <jesse.brandeburg@...el.com>;
>>> intel-wired-lan@...ts.osuosl.org; Ruhl, Michael J <michael.j.ruhl@...el.com>;
>>> Keller, Jacob E <jacob.e.keller@...el.com>; G, GurucharanX
>>> <gurucharanx.g@...el.com>
>>> Subject: Re: [PATCH net-next 5/6] igb: Do not free q_vector unless new one
>>> was allocated
>>>
>>> On Fri, Nov 04, 2022 at 01:54:13PM -0700, Tony Nguyen wrote:
>>>> From: Kees Cook <keescook@...omium.org>
>>>>
>>>> Avoid potential use-after-free condition under memory pressure. If the
>>>> kzalloc() fails, q_vector will be freed but left in the original
>>>> adapter->q_vector[v_idx] array position.
>>>>
>>>> Cc: Jesse Brandeburg <jesse.brandeburg@...el.com>
>>>> Cc: Tony Nguyen <anthony.l.nguyen@...el.com>
>>>> Cc: "David S. Miller" <davem@...emloft.net>
>>>> Cc: Eric Dumazet <edumazet@...gle.com>
>>>> Cc: Jakub Kicinski <kuba@...nel.org>
>>>> Cc: Paolo Abeni <pabeni@...hat.com>
>>>> Cc: intel-wired-lan@...ts.osuosl.org
>>>> Cc: netdev@...r.kernel.org
>>>> Signed-off-by: Kees Cook <keescook@...omium.org>
>>>> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@...el.com>
>>>> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
>>>> Tested-by: Gurucharan <gurucharanx.g@...el.com> (A Contingent worker
>>> at Intel)
>>>
>>> You should use first and last names here.
>>>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>>>> ---
>>>>   drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
>>> b/drivers/net/ethernet/intel/igb/igb_main.c
>>>> index d6c1c2e66f26..c2bb658198bf 100644
>>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>>> @@ -1202,8 +1202,12 @@ static int igb_alloc_q_vector(struct igb_adapter
>>> *adapter,
>>>>   	if (!q_vector) {
>>>>   		q_vector = kzalloc(size, GFP_KERNEL);
>>>>   	} else if (size > ksize(q_vector)) {
>>>> -		kfree_rcu(q_vector, rcu);
>>>> -		q_vector = kzalloc(size, GFP_KERNEL);
>>>> +		struct igb_q_vector *new_q_vector;
>>>> +
>>>> +		new_q_vector = kzalloc(size, GFP_KERNEL);
>>>> +		if (new_q_vector)
>>>> +			kfree_rcu(q_vector, rcu);
>>>> +		q_vector = new_q_vector;
>>>
>>> I wonder if this is correct.
>>> 1. if new_q_vector is NULL, you will overwrite q_vector without releasing it.
>>> 2. kfree_rcu() doesn't immediately release memory, but after grace
>>> period, but here you are overwriting the pointer which is not release
>>> yet.
>>
>> The actual pointer is: adapter->q_vector[v_idx]
>>
>> q_vector is just a convenience pointer.
>>
>> If the allocation succeeds, the q_vector[v_idx] will be replaced (later in the code).
>>
>> If the allocation fails, this is not being freed.  The original code freed the adapter
>> pointer but didn't not remove the pointer.
>>
>> If q_vector is NULL,  (i.e. the allocation failed), the function exits, but the original
>> pointer is left in place.
>>
>> I think this logic is correct.
>>
>> The error path leaves the original allocation in place.  If this is incorrect behavior,
>> a different change would be:
>>
>> 	q_vector = adapter->q_vector[v_idx];
>> 	adapter->q_vector[v_idx] = NULL;
>> 	... the original code...
>>
>> But I am not sure if that is what is desired?
> 
> I understand the issue what you are trying to solve, I just don't
> understand your RCU code. I would expect calls to rcu_dereference()
> in order to get q_vector and rcu_assign_pointer() to clear
> adapter->q_vector[v_idx], but igb has none.
> 
> Thanks

the uses of kfree_rcu were introduced by 5536d2102a2d ("igb: Combine 
q_vector and ring allocation into a single function")

The commit doesn't mention switching from kfree to kfree_rcu and I 
suspect that the igb driver is not actually really using RCU semantics 
properly.

The closest explanation is that the get_stats64 function might be 
accessing the ring and thus needs the RCU grace period.. but I think 
you're right in that we're missing the necessary RCU access macros.

Thanks,
Jake