| 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
| ||
|
Date: Thu, 17 Aug 2023 14:39:56 +0200
From: Alejandro Colomar <alx@...nel.org>
To: Kees Cook <keescook@...omium.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
linux-hardening@...r.kernel.org
Subject: Re: struct_size() using sizeof() vs offsetof()
Hi!
On 2023-08-17 05:05, Kees Cook wrote:
> On Thu, Aug 17, 2023 at 02:23:21AM +0200, Alejandro Colomar wrote:
[...]
>
> When struct_size() was originally implemented this topic came up, and we
> opted for potential over-estimation rather than using offsetof() which
> could result in under-allocation, and using max() of two different
> calculations just seemed like overkill. Additionally, almost all cases of
> struct_size() was replacing a literal open-coded version of
>
> sizeof(*ptr) + sizeof(*ptr->array) * count
>
> So avoiding a difference in calculation was nice too.
Yup.
[...]
>>
>> MAX(sizeof(s), offsetof(s, fam) + sizeof_member(s, fam) * count)
>
> Ironically, this has been under careful examination recently by GCC[1]
> too. Though that has mainly been looking at it from the perspective
> of how __builtin_object_size() should behave in the face of the new
> __counted_by attribute.
Heh, I've found that there are actually a lot of discussions about flex
arrays going on this summer. Glibc also has something.
[...]
>
> We opted for simple over complex, with the understanding that
> over-allocation will be a relatively rare issue that will only waste
> limited space (as opposed to potential under-allocation and risking
> writing beyond the end of the region).
Thanks.
[...]
> But, yes, at the end of the day, struct_size() could be defined as
> max(sizeof, offsetof-based struct-size).
>
> Note that struct_size() has been designed to have two additional
> behaviors:
> - be usable as a constant expression
> - saturate at SIZE_MAX
>
> So as long as the max() could do the same (which it should be able to),
> it'd likely be fine.
Yep. It should be able to do that.
> I'm open to patches as long as we can validate any
> binary differences found in allmodconfig builds. :)
Thanks! I'm preparing a patch. It's being more complex than I thought
it would be. There's some thing that's not compiling for me.
net/sched/cls_u32.c: In function ‘u32_init’:
net/sched/cls_u32.c:369:17: error: cannot apply ‘offsetof’ to a non constant address
369 | tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL);
| ^~~~
In file included from ./include/linux/kernel.h:27,
from ./arch/x86/include/asm/percpu.h:27,
from ./arch/x86/include/asm/nospec-branch.h:14,
from ./arch/x86/include/asm/paravirt_types.h:27,
from ./arch/x86/include/asm/ptrace.h:97,
from ./arch/x86/include/asm/math_emu.h:5,
from ./arch/x86/include/asm/processor.h:13,
from ./arch/x86/include/asm/timex.h:5,
from ./include/linux/timex.h:67,
from ./include/linux/time32.h:13,
from ./include/linux/time.h:60,
from ./include/linux/stat.h:19,
from ./include/linux/module.h:13,
from net/sched/cls_u32.c:26:
Here's the entire function:
$ grepc u32_init
./net/sched/cls_u32.c:352:
static int u32_init(struct tcf_proto *tp)
{
struct tc_u_hnode *root_ht;
void *key = tc_u_common_ptr(tp);
struct tc_u_common *tp_c = tc_u_common_find(key);
root_ht = kzalloc(struct_size(root_ht, ht, 1), GFP_KERNEL);
if (root_ht == NULL)
return -ENOBUFS;
root_ht->refcnt++;
root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
root_ht->prio = tp->prio;
root_ht->is_root = true;
idr_init(&root_ht->handle_idr);
if (tp_c == NULL) {
tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL);
if (tp_c == NULL) {
kfree(root_ht);
return -ENOBUFS;
}
tp_c->ptr = key;
INIT_HLIST_NODE(&tp_c->hnode);
idr_init(&tp_c->handle_idr);
hlist_add_head(&tp_c->hnode, tc_u_hash(key));
}
tp_c->refcnt++;
RCU_INIT_POINTER(root_ht->next, tp_c->hlist);
rcu_assign_pointer(tp_c->hlist, root_ht);
root_ht->refcnt++;
rcu_assign_pointer(tp->root, root_ht);
tp->data = tp_c;
return 0;
}
Let's see the structure type:
$ grepc -tt tc_u_common
./net/sched/cls_u32.c:86:
struct tc_u_common {
struct tc_u_hnode __rcu *hlist;
void *ptr;
int refcnt;
struct idr handle_idr;
struct hlist_node hnode;
long knodes;
};
Huh, hlist is the first field and is a pointer. I'm not at all
sure of what was being done here. Here's the type of the
pointee:
$ grepc -tt tc_u_hnode
./net/sched/cls_u32.c:70:
struct tc_u_hnode {
struct tc_u_hnode __rcu *next;
u32 handle;
u32 prio;
int refcnt;
unsigned int divisor;
struct idr handle_idr;
bool is_root;
struct rcu_head rcu;
u32 flags;
/* The 'ht' field MUST be the last field in structure to allow for
* more entries allocated at end of structure.
*/
struct tc_u_knode __rcu *ht[];
};
So, that struct_size() was, at best, doing black magic. At worst, a
bug. I would need to investigate that code a little bit more, but a
first guess tells me that struct_size() was returning the size of the
outer structure plus the size of the flex array in the inner structure,
but not including the size of the inner structure; i.e.:
sizeof(outer) + flex_array_size(inner-flex)
which seems a weird calcualtion.
That line of code was written by Gustavo, in d61491a51f7e ("net/sched:
cls_u32: Replace one-element array with flexible-array member"), so
can you please confirm that code, and maybe explain why it's that way,
Gustavo?
- tp_c = kzalloc(sizeof(*tp_c), GFP_KERNEL);
+ tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL);
Cheers,
Alex
>
> -Kees
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626672.html
>
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists