[v2,06/16] igb: Proactively round up to kmalloc bucket size

Commit Message

Kees Cook Sept. 23, 2022, 8:28 p.m. UTC

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().

Additionally fix potential use-after-free in the case of new allocation
failure: only free memory if the replacement allocation succeeds.

Cc: Jesse Brandeburg <[e-mail]>
Cc: Tony Nguyen <[e-mail]>
Cc: "David S. Miller" <[e-mail]>
Cc: Eric Dumazet <[e-mail]>
Cc: Jakub Kicinski <[e-mail]>
Cc: Paolo Abeni <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Kees Cook <[e-mail]>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Michael J. Ruhl Sept. 26, 2022, 3:49 p.m. UTC | #1

>-----Original Message-----
>From: Kees Cook <[e-mail]>
>Sent: Friday, September 23, 2022 4:28 PM
>To: Vlastimil Babka <[e-mail]>
>Cc: Kees Cook <[e-mail]>; Brandeburg, Jesse
><[e-mail]>; Nguyen, Anthony L
><[e-mail]>; David S. Miller <[e-mail]>;
>Eric Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>;
>Paolo Abeni <[e-mail]>; [e-mail];
>[e-mail]; Ruhl, Michael J <[e-mail]>;
>Hyeonggon Yoo <[e-mail]>; Christoph Lameter
><[e-mail]>; Pekka Enberg <[e-mail]>; David Rientjes
><[e-mail]>; Joonsoo Kim <[e-mail]>; Andrew
>Morton <[e-mail]>; Greg Kroah-Hartman
><[e-mail]>; Nick Desaulniers
><[e-mail]>; Alex Elder <[e-mail]>; Josef Bacik
><[e-mail]>; David Sterba <[e-mail]>; Sumit Semwal
><[e-mail]>; Christian König <[e-mail]>;
>Daniel Micay <[e-mail]>; Yonghong Song <[e-mail]>;
>Marco Elver <[e-mail]>; Miguel Ojeda <[e-mail]>; linux-
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; linaro-mm-
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail]
>Subject: [PATCH v2 06/16] igb: Proactively round up to kmalloc bucket size
>
>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().
>
>Additionally fix potential use-after-free in the case of new allocation
>failure: only free memory if the replacement allocation succeeds.
>
>Cc: Jesse Brandeburg <[e-mail]>
>Cc: Tony Nguyen <[e-mail]>
>Cc: "David S. Miller" <[e-mail]>
>Cc: Eric Dumazet <[e-mail]>
>Cc: Jakub Kicinski <[e-mail]>
>Cc: Paolo Abeni <[e-mail]>
>Cc: [e-mail]
>Cc: [e-mail]
>Signed-off-by: Kees Cook <[e-mail]>
>---
> drivers/net/ethernet/intel/igb/igb_main.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
>b/drivers/net/ethernet/intel/igb/igb_main.c
>index 2796e81d2726..eb51e531c096 100644
>--- a/drivers/net/ethernet/intel/igb/igb_main.c
>+++ b/drivers/net/ethernet/intel/igb/igb_main.c
>@@ -1195,15 +1195,16 @@ 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));

This looks good to me...

> 	/* allocate q_vector and rings */
> 	q_vector = adapter->q_vector[v_idx];
> 	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);
>+		if (q_vector)
>+			kfree_rcu(q_vector, rcu);

Even though this is in the ksize part, this seems like an unrelated change?
 Should this be in a different patch?

Also, the kfree_rcu will free q_vector after the RCU grace period?

Is that what you want to do?

How does rcu distinguish between the original q_vector, and the newly kzalloced one?

Thanks,

Mike



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

Patch

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 2796e81d2726..eb51e531c096 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1195,15 +1195,16 @@  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];
 	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);
+		if (q_vector)
+			kfree_rcu(q_vector, rcu);
 	} else {
 		memset(q_vector, 0, size);
 	}