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

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

[Tony Nguyen]

From: Kees Cook <keescook@chromium.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@intel.com>
Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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;
 	} else {
 		memset(q_vector, 0, size);
 	}
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Leon Romanovsky]

On Fri, Nov 04, 2022 at 01:54:13PM -0700, Tony Nguyen wrote:
> From: Kees Cook <keescook@chromium.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@intel.com>
> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)

You should use first and last names here.

> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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.


>  	} else {
>  		memset(q_vector, 0, size);
>  	}
> -- 
> 2.35.1
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Ruhl, Michael J]

>-----Original Message-----
>From: Leon Romanovsky <leon@kernel.org>
>Sent: Monday, November 7, 2022 2:03 AM
>To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
>Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>edumazet@google.com; Kees Cook <keescook@chromium.org>;
>netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>;
>intel-wired-lan@lists.osuosl.org; Ruhl, Michael J <michael.j.ruhl@intel.com>;
>Keller, Jacob E <jacob.e.keller@intel.com>; G, GurucharanX
><gurucharanx.g@intel.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@chromium.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@intel.com>
>> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Cc: intel-wired-lan@lists.osuosl.org
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker
>at Intel)
>
>You should use first and last names here.
>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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?

Mike

>
>>  	} else {
>>  		memset(q_vector, 0, size);
>>  	}
>> --
>> 2.35.1
>>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Leon Romanovsky]

On Mon, Nov 07, 2022 at 01:55:58PM +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Leon Romanovsky <leon@kernel.org>
> >Sent: Monday, November 7, 2022 2:03 AM
> >To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> >Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> >edumazet@google.com; Kees Cook <keescook@chromium.org>;
> >netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>;
> >intel-wired-lan@lists.osuosl.org; Ruhl, Michael J <michael.j.ruhl@intel.com>;
> >Keller, Jacob E <jacob.e.keller@intel.com>; G, GurucharanX
> ><gurucharanx.g@intel.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@chromium.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@intel.com>
> >> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
> >> Cc: "David S. Miller" <davem@davemloft.net>
> >> Cc: Eric Dumazet <edumazet@google.com>
> >> Cc: Jakub Kicinski <kuba@kernel.org>
> >> Cc: Paolo Abeni <pabeni@redhat.com>
> >> Cc: intel-wired-lan@lists.osuosl.org
> >> Cc: netdev@vger.kernel.org
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> >> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker
> >at Intel)
> >
> >You should use first and last names here.
> >
> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Jacob Keller]

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@kernel.org>
>>> Sent: Monday, November 7, 2022 2:03 AM
>>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
>>> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>>> edumazet@google.com; Kees Cook <keescook@chromium.org>;
>>> netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>;
>>> intel-wired-lan@lists.osuosl.org; Ruhl, Michael J <michael.j.ruhl@intel.com>;
>>> Keller, Jacob E <jacob.e.keller@intel.com>; G, GurucharanX
>>> <gurucharanx.g@intel.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@chromium.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@intel.com>
>>>> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>> Cc: "David S. Miller" <davem@davemloft.net>
>>>> Cc: Eric Dumazet <edumazet@google.com>
>>>> Cc: Jakub Kicinski <kuba@kernel.org>
>>>> Cc: Paolo Abeni <pabeni@redhat.com>
>>>> Cc: intel-wired-lan@lists.osuosl.org
>>>> Cc: netdev@vger.kernel.org
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>>>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>>>> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker
>>> at Intel)
>>>
>>> You should use first and last names here.
>>>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Jakub Kicinski]

On Mon, 7 Nov 2022 10:35:14 -0800 Jacob Keller wrote:
> > 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.
> 
> 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.

Alright, expecting a follow up for this.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 6/6] igb: Proactively round up to kmalloc bucket size

[Tony Nguyen]

From: Kees Cook <keescook@chromium.org>

In preparation for removing the "silently change allocation size"
users of ksize(), explicitly round up all q_vector allocations so that
allocations can be correctly compared to ksize().

Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index c2bb658198bf..97290fc0fddd 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1195,7 +1195,7 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter,
 		return -ENOMEM;
 
 	ring_count = txr_count + rxr_count;
-	size = struct_size(q_vector, ring, ring_count);
+	size = kmalloc_size_roundup(struct_size(q_vector, ring, ring_count));
 
 	/* allocate q_vector and rings */
 	q_vector = adapter->q_vector[v_idx];
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan