RE: [PATCH v3 1/2] igb: Do not free q_vector unless new one was allocated

[Ruhl, Michael J]

>-----Original Message-----
>From: Kees Cook <keescook@xxxxxxxxxxxx>
>Sent: Tuesday, October 18, 2022 5:25 AM
>To: Ruhl@xxxxxxxxxxxxxxx; Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>
>Cc: Kees Cook <keescook@xxxxxxxxxxxx>; Brandeburg, Jesse
><jesse.brandeburg@xxxxxxxxx>; Nguyen, Anthony L
><anthony.l.nguyen@xxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>;
>Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
>Paolo Abeni <pabeni@xxxxxxxxxx>; intel-wired-lan@xxxxxxxxxxxxxxxx;
>netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
>hardening@xxxxxxxxxxxxxxx
>Subject: [PATCH v3 1/2] igb: Do not free q_vector unless new one was
>allocated
>
>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@xxxxxxxxx>
>Cc: Tony Nguyen <anthony.l.nguyen@xxxxxxxxx>
>Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
>Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
>Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
>Cc: Paolo Abeni <pabeni@xxxxxxxxxx>
>Cc: intel-wired-lan@xxxxxxxxxxxxxxxx
>Cc: netdev@xxxxxxxxxxxxxxx
>Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>---
> 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 f8e32833226c..6256855d0f62 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;

Ok, that makes more sense to me.

Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>

Mike


> 	} else {
> 		memset(q_vector, 0, size);
> 	}
>--
>2.34.1