* [PATCH v2] net: sched: cls_u32: Fix allocation size in u32_init()
@ 2023-10-04 13:19 Gustavo A. R. Silva
2023-10-04 15:16 ` Jamal Hadi Salim
2023-10-06 10:50 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2023-10-04 13:19 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Gustavo A. R. Silva
Cc: netdev, linux-kernel, Gustavo A. R. Silva, linux-hardening,
Kees Cook
commit d61491a51f7e ("net/sched: cls_u32: Replace one-element array
with flexible-array member") incorrecly replaced an instance of
`sizeof(*tp_c)` with `struct_size(tp_c, hlist->ht, 1)`. This results
in a an over-allocation of 8 bytes.
This change is wrong because `hlist` in `struct tc_u_common` is a
pointer:
net/sched/cls_u32.c:
struct tc_u_common {
struct tc_u_hnode __rcu *hlist;
void *ptr;
int refcnt;
struct idr handle_idr;
struct hlist_node hnode;
long knodes;
};
So, the use of `struct_size()` makes no sense: we don't need to allocate
any extra space for a flexible-array member. `sizeof(*tp_c)` is just fine.
So, `struct_size(tp_c, hlist->ht, 1)` translates to:
sizeof(*tp_c) + sizeof(tp_c->hlist->ht) ==
sizeof(struct tc_u_common) + sizeof(struct tc_u_knode *) ==
144 + 8 == 0x98 (byes)
^^^
|
unnecessary extra
allocation size
$ pahole -C tc_u_common net/sched/cls_u32.o
struct tc_u_common {
struct tc_u_hnode * hlist; /* 0 8 */
void * ptr; /* 8 8 */
int refcnt; /* 16 4 */
/* XXX 4 bytes hole, try to pack */
struct idr handle_idr; /* 24 96 */
/* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
struct hlist_node hnode; /* 120 16 */
/* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */
long int knodes; /* 136 8 */
/* size: 144, cachelines: 3, members: 6 */
/* sum members: 140, holes: 1, sum holes: 4 */
/* last cacheline: 16 bytes */
};
And with `sizeof(*tp_c)`, we have:
sizeof(*tp_c) == sizeof(struct tc_u_common) == 144 == 0x90 (bytes)
which is the correct and original allocation size.
Fix this issue by replacing `struct_size(tp_c, hlist->ht, 1)` with
`sizeof(*tp_c)`, and avoid allocating 8 too many bytes.
The following difference in binary output is expected and reflects the
desired change:
| net/sched/cls_u32.o
| @@ -6148,7 +6148,7 @@
| include/linux/slab.h:599
| 2cf5: mov 0x0(%rip),%rdi # 2cfc <u32_init+0xfc>
| 2cf8: R_X86_64_PC32 kmalloc_caches+0xc
|- 2cfc: mov $0x98,%edx
|+ 2cfc: mov $0x90,%edx
Reported-by: Alejandro Colomar <alx@kernel.org>
Closes: https://lore.kernel.org/lkml/09b4a2ce-da74-3a19-6961-67883f634d98@kernel.org/
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
- Update subject line.
- Update changelog text.
v1:
- Link: https://lore.kernel.org/linux-hardening/ZN5DvRyq6JNz20l1@work/
net/sched/cls_u32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index da4c179a4d41..6663e971a13e 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -366,7 +366,7 @@ static int u32_init(struct tcf_proto *tp)
idr_init(&root_ht->handle_idr);
if (tp_c == NULL) {
- tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL);
+ tp_c = kzalloc(sizeof(*tp_c), GFP_KERNEL);
if (tp_c == NULL) {
kfree(root_ht);
return -ENOBUFS;
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] net: sched: cls_u32: Fix allocation size in u32_init()
2023-10-04 13:19 [PATCH v2] net: sched: cls_u32: Fix allocation size in u32_init() Gustavo A. R. Silva
@ 2023-10-04 15:16 ` Jamal Hadi Salim
2023-10-06 10:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Jamal Hadi Salim @ 2023-10-04 15:16 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Cong Wang, Jiri Pirko, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
linux-hardening, Kees Cook
On Wed, Oct 4, 2023 at 9:19 AM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> commit d61491a51f7e ("net/sched: cls_u32: Replace one-element array
> with flexible-array member") incorrecly replaced an instance of
> `sizeof(*tp_c)` with `struct_size(tp_c, hlist->ht, 1)`. This results
> in a an over-allocation of 8 bytes.
>
> This change is wrong because `hlist` in `struct tc_u_common` is a
> pointer:
>
> net/sched/cls_u32.c:
> struct tc_u_common {
> struct tc_u_hnode __rcu *hlist;
> void *ptr;
> int refcnt;
> struct idr handle_idr;
> struct hlist_node hnode;
> long knodes;
> };
>
> So, the use of `struct_size()` makes no sense: we don't need to allocate
> any extra space for a flexible-array member. `sizeof(*tp_c)` is just fine.
>
> So, `struct_size(tp_c, hlist->ht, 1)` translates to:
>
> sizeof(*tp_c) + sizeof(tp_c->hlist->ht) ==
> sizeof(struct tc_u_common) + sizeof(struct tc_u_knode *) ==
> 144 + 8 == 0x98 (byes)
> ^^^
> |
> unnecessary extra
> allocation size
>
> $ pahole -C tc_u_common net/sched/cls_u32.o
> struct tc_u_common {
> struct tc_u_hnode * hlist; /* 0 8 */
> void * ptr; /* 8 8 */
> int refcnt; /* 16 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct idr handle_idr; /* 24 96 */
> /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
> struct hlist_node hnode; /* 120 16 */
> /* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */
> long int knodes; /* 136 8 */
>
> /* size: 144, cachelines: 3, members: 6 */
> /* sum members: 140, holes: 1, sum holes: 4 */
> /* last cacheline: 16 bytes */
> };
>
> And with `sizeof(*tp_c)`, we have:
>
> sizeof(*tp_c) == sizeof(struct tc_u_common) == 144 == 0x90 (bytes)
>
> which is the correct and original allocation size.
>
> Fix this issue by replacing `struct_size(tp_c, hlist->ht, 1)` with
> `sizeof(*tp_c)`, and avoid allocating 8 too many bytes.
>
> The following difference in binary output is expected and reflects the
> desired change:
>
> | net/sched/cls_u32.o
> | @@ -6148,7 +6148,7 @@
> | include/linux/slab.h:599
> | 2cf5: mov 0x0(%rip),%rdi # 2cfc <u32_init+0xfc>
> | 2cf8: R_X86_64_PC32 kmalloc_caches+0xc
> |- 2cfc: mov $0x98,%edx
> |+ 2cfc: mov $0x90,%edx
>
> Reported-by: Alejandro Colomar <alx@kernel.org>
> Closes: https://lore.kernel.org/lkml/09b4a2ce-da74-3a19-6961-67883f634d98@kernel.org/
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
> Changes in v2:
> - Update subject line.
> - Update changelog text.
>
> v1:
> - Link: https://lore.kernel.org/linux-hardening/ZN5DvRyq6JNz20l1@work/
>
> net/sched/cls_u32.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index da4c179a4d41..6663e971a13e 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -366,7 +366,7 @@ static int u32_init(struct tcf_proto *tp)
> idr_init(&root_ht->handle_idr);
>
> if (tp_c == NULL) {
> - tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL);
> + tp_c = kzalloc(sizeof(*tp_c), GFP_KERNEL);
> if (tp_c == NULL) {
> kfree(root_ht);
> return -ENOBUFS;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] net: sched: cls_u32: Fix allocation size in u32_init()
2023-10-04 13:19 [PATCH v2] net: sched: cls_u32: Fix allocation size in u32_init() Gustavo A. R. Silva
2023-10-04 15:16 ` Jamal Hadi Salim
@ 2023-10-06 10:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-10-06 10:50 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, linux-hardening, keescook
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:
On Wed, 4 Oct 2023 15:19:37 +0200 you wrote:
> commit d61491a51f7e ("net/sched: cls_u32: Replace one-element array
> with flexible-array member") incorrecly replaced an instance of
> `sizeof(*tp_c)` with `struct_size(tp_c, hlist->ht, 1)`. This results
> in a an over-allocation of 8 bytes.
>
> This change is wrong because `hlist` in `struct tc_u_common` is a
> pointer:
>
> [...]
Here is the summary with links:
- [v2] net: sched: cls_u32: Fix allocation size in u32_init()
https://git.kernel.org/netdev/net/c/c4d49196ceec
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-06 10:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-04 13:19 [PATCH v2] net: sched: cls_u32: Fix allocation size in u32_init() Gustavo A. R. Silva
2023-10-04 15:16 ` Jamal Hadi Salim
2023-10-06 10:50 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).