linux-hardening - Re: [PATCH v2] net: sched: cls_u32: Fix allocation size in u32

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 4 Oct 2023 11:16:50 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org, 
	Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH v2] net: sched: cls_u32: Fix allocation size in u32_init()

On Wed, Oct 4, 2023 at 9:19 AM Gustavo A. R. Silva
<gustavoars@...nel.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@...nel.org>
> Closes: https://lore.kernel.org/lkml/09b4a2ce-da74-3a19-6961-67883f634d98@kernel.org/
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>

Acked-by: Jamal Hadi Salim <jhs@...atatu.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
>