igb: Proactively round up to kmalloc bucket size
Subsystems (3)
v3 cover igb: Proactively round up to kmalloc bucket size
Hi,
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().
Before that, fix a potential Use After Free under memory pressure.
Thanks,
-Kees
v3; split UAF fix from bucket rounding.
v2: https://lore.kernel.org/lkml/[email protected]/
Kees Cook (2):
igb: Do not free q_vector unless new one was allocated
igb: Proactively round up to kmalloc bucket size
drivers/net/ethernet/intel/igb/igb_main.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
Signature · 6 lines
2.34.1 _______________________________________________ Intel-wired-lan mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
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.
--- drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/igb/igb_main.c +6 -2
@@ -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); }
Signature · 6 lines
2.34.1 _______________________________________________ Intel-wired-lan mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
RE: [PATCH v3 1/2] igb: Do not free q_vector unless new one was allocated
Quoted 39 lines
-----Original Message-----
From: Kees Cook <[email protected]>
Sent: Tuesday, October 18, 2022 5:25 AM
To: [email protected]; Ruhl, Michael J <[email protected]>
Cc: Kees Cook <[email protected]>; Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: [PATCH v3 1/2] igb: Do not freeq_vectorunless new one was allocatedAvoid potential use-after-free condition under memory pressure. If the
kzalloc()fails,q_vectorwill be freed but left in the original adapter->q_vector[v_idx] array position.Cc Jesse Brandeburg <[email protected]>Cc Tony Nguyen <[email protected]>Cc "David S. Miller" <[email protected]>Cc Eric Dumazet <[email protected]>Cc Jakub Kicinski <[email protected]>Cc Paolo Abeni <[email protected]>Signed-off-by Kees Cook <[email protected]>---
drivers/net/ethernet/intel/igb/igb_main.c| 8 ++++++--1 file changed, 6 insertions(+), 2 deletions(-)drivers/net/ethernet/intel/igb/igb_main.c +0
-0@@ -1202,8 +1202,12 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter, if (!q_vector) {
drivers/net/ethernet/intel/igb/igb_main.c static int igb_alloc_q_vector(struct igb_adapter · line 1211
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_main.c +6
-2q_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.
Mike
Quoted 5 lines
drivers/net/ethernet/intel/igb/igb_main.c +0
-0}else {memset (q_vector,0 , size);}--
2.34.1
_______________________________________________
Intel-wired-lan mailing list
[email protected]
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Jacob Keller
Reviewed-byRe: [Intel-wired-lan] [PATCH v3 1/2] igb: Do not free q_vector unless new one was allocated
Quoted 41 lines · Michael J
Quoted 41 lines
-----Original Message-----
From: Kees Cook <[email protected]>
Sent: Tuesday, October 18, 2022 5:25 AM
To: [email protected]; Ruhl, Michael J <[email protected]>
Cc: Kees Cook <[email protected]>; Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: [PATCH v3 1/2] igb: Do not freeq_vectorunless new one was allocatedAvoid potential use-after-free condition under memory pressure. If the
kzalloc()fails,q_vectorwill be freed but left in the original adapter->q_vector[v_idx] array position.Cc Jesse Brandeburg <[email protected]>Cc Tony Nguyen <[email protected]>Cc "David S. Miller" <[email protected]>Cc Eric Dumazet <[email protected]>Cc Jakub Kicinski <[email protected]>Cc Paolo Abeni <[email protected]>Signed-off-by Kees Cook <[email protected]>---
drivers/net/ethernet/intel/igb/igb_main.c| 8 ++++++--1 file changed, 6 insertions(+), 2 deletions(-)drivers/net/ethernet/intel/igb/igb_main.c +0
-0@@ -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)) {
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_main.c static int igb_alloc_q_vector(struct igb_adapter *adapter, · line 1210
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_main.c +6
-2-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;
Fix seems straight forward.
This feels like something we could tag with a Fixes and send through net?
Thanks,
Jake
_______________________________________________
Intel-wired-lan mailing list
[email protected]
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
RE: [Intel-wired-lan] [PATCH v3 1/2] igb: Do not free q_vector unless new one was allocated
In reply to Kees CookQuoted 28 lines
-----Original Message-----
From: Intel-wired-lan <[email protected]> On Behalf Of Kees Cook
Sent: Tuesday, October 18, 2022 2:55 PM
To: [email protected]; Ruhl, Michael J <[email protected]>
Cc: Kees Cook <[email protected]>; [email protected]; [email protected]; Eric Dumazet <[email protected]>; [email protected]; [email protected]; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; David S. Miller <[email protected]>
Subject: [Intel-wired-lan] [PATCH v3 1/2] igb: Do not freeq_vectorunless new one was allocatedAvoid potential use-after-free condition under memory pressure. If the
kzalloc()fails,q_vectorwill be freed but left in the original adapter->q_vector[v_idx] array position.Cc Jesse Brandeburg <[email protected]>Cc Tony Nguyen <[email protected]>Cc "David S. Miller" <[email protected]>Cc Eric Dumazet <[email protected]>Cc Jakub Kicinski <[email protected]>Cc Paolo Abeni <[email protected]>Signed-off-by Kees Cook <[email protected]>
Quoted 4 lines
--- drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
PATCH v3 2/2 igb: Proactively round up to kmalloc bucket size
In reply to Kees CookIn 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().
--- drivers/net/ethernet/intel/igb/igb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/intel/igb/igb_main.c +1 -1
@@ -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];
Signature · 6 lines
2.34.1 _______________________________________________ Intel-wired-lan mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Quoted 5 lines · Kees Cook
In preparation for removing the "silently change allocation size" users of
ksize(), explicitly round up allq_vectorallocations so that allocations can be correctly compared toksize().Signed-off-by Kees Cook <[email protected]>
Hi! Any feedback on this part of the patch pair?
Quoted 9 lines
--- drivers/net/ethernet/intel/igb/igb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/ethernet/intel/igb/igb_main.c +0
-0
drivers/net/ethernet/intel/igb/igb_main.c static int igb_alloc_q_vector(struct igb_adapter *adapter, · line 1201
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_main.c +1
-1@@ -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];
Thanks! :)
-Kees
Signature · 1 line
Kees Cook
Quoted 32 lines
-----Original Message-----
From: Kees Cook <[email protected]>
Sent: Friday, October 28, 2022 11:18 PM
To: Ruhl, Michael J <[email protected]>
Cc: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 2/2] igb: Proactively round up to kmalloc bucket sizeQuoted 5 lines · Kees Cook
In preparation for removing the "silently change allocation size" users of
ksize(), explicitly round up allq_vectorallocations so that allocations can be correctly compared toksize().Signed-off-by Kees Cook <[email protected]>Hi! Any feedback on this part of the patch pair?
Review
Quoted 5 lines
--- drivers/net/ethernet/intel/igb/igb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/ethernet/intel/igb/igb_main.cdrivers/net/ethernet/intel/igb/igb_main.c static int igb_alloc_q_vector(struct igb_adapter
Quoted 4 lines
index
6256855d0f62..7a3a41dc0276100644drivers/net/ethernet/intel/igb/igb_main.c +0
-0@@ -1195,7 +1195,7 @@ static int igb_alloc_q_vector(struct igb_adapter*adapter,
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_main.c static int igb_alloc_q_vector(struct igb_adapter *adapter, · line 1201
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_main.c +1
-1return -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];
Hi Kees,
Looking at the size usage (from elixir), I see:
--
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);
} else {
memset(q_vector, 0, size);
}--
If the size is rounded up, will the (size > ksize()) check ever be true?
I.e. have you eliminated this check (and maybe getting rid of the need for first patch?)?
Thanks,
Mike
Quoted 7 lines
Thanks! :)
-Kees
--
Kees Cook
Quoted 11 lines · Michael J
Looking at the size usage (from elixir), I see:
--
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); } else { memset(q_vector, 0, size); }
Quoted 5 lines
--
If the size is rounded up, will the (size >
ksize()) check ever be true?I.e. have you eliminated this check (and maybe getting rid of the need for first patch?)?
Hi!
It looked like igb_alloc_q_vector() was designed to be called multiple times on the same q_vector (i.e. to grow its allocation size over time). So for that case, yes, the "size > ksize(q_vector)" check is needed. If it's only ever called once (which is hard for me to tell), then no. (And if "no", why was the alloc/free case even there in the first place?)
-Kees
Signature · 1 line
Kees Cook
Ruhl, Michael J
Reviewed-byQuoted 31 lines
-----Original Message-----
From: Kees Cook <[email protected]>
Sent: Tuesday, November 1, 2022 5:37 PM
To: Ruhl, Michael J <[email protected]>
Cc: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 2/2] igb: Proactively round up to kmalloc bucket sizeQuoted 11 lines · Michael J
Looking at the size usage (from elixir), I see:
--
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); } else { memset(q_vector, 0, size); }Quoted 5 lines
--
If the size is rounded up, will the (size >
ksize()) check ever be true?I.e. have you eliminated this check (and maybe getting rid of the need for
first patch?)?
Quoted 7 lines
Hi!
It looked like
igb_alloc_q_vector()was designed to be called multiple times on the sameq_vector(i.e. to grow its allocation size over time). So for that case, yes, the "size >ksize(q_vector)" check is needed. If it's only ever called once (which is hard for me to tell), then no. (And if "no", why was the alloc/free case even there in the first place?)
Ahh, Ok, I missed the fact that size is based on ring_count. When I saw the "struct_size" I assumed that size would be the same every time and missed this point.
So can vary over time, and this ksize check is needed.
With that in mind these patches look good to me.
Mike
Quoted 4 lines
-Kees
--
Kees Cook
_______________________________________________
Intel-wired-lan mailing list
[email protected]
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Quoted 28 lines
-----Original Message-----
From: Intel-wired-lan <[email protected]> On Behalf Of Kees Cook
Sent: Tuesday, October 18, 2022 2:55 PM
To: [email protected]; Ruhl, Michael J <[email protected]>
Cc: Kees Cook <[email protected]>; [email protected]; [email protected]; Eric Dumazet <[email protected]>; [email protected]; [email protected]; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; David S. Miller <[email protected]>
Subject: [Intel-wired-lan] [PATCH v3 2/2] igb: Proactively round up to kmalloc bucket sizeIn preparation for removing the "silently change allocation size" users of
ksize(), explicitly round up allq_vectorallocations so that allocations can be correctly compared toksize().Cc Jesse Brandeburg <[email protected]>Cc Tony Nguyen <[email protected]>Cc "David S. Miller" <[email protected]>Cc Eric Dumazet <[email protected]>Cc Jakub Kicinski <[email protected]>Cc Paolo Abeni <[email protected]>Signed-off-by Kees Cook <[email protected]>
Quoted 4 lines
--- drivers/net/ethernet/intel/igb/igb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
_______________________________________________
Intel-wired-lan mailing list
[email protected]
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Quoted 6 lines · Kees Cook
Kees Cook (2):
igb: Do not freeq_vectorunless new one was allocated
igb: Proactively round up to kmalloc bucket sizedrivers/net/ethernet/intel/igb/igb_main.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
Ugh, yay for my MUA vs commas. Sorry for any future typo bounce spam. :(
Signature · 5 lines
Kees Cook _______________________________________________ Intel-wired-lan mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/intel-wired-lan