| Message ID | 20211214114854.133328-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Gustavo A. R. Silva |
| Headers | show |
| Series | [-next] audit: use struct_size() helper in kmalloc() | expand |
On Tue, Dec 14, 2021 at 07:48:54PM +0800, Xiu Jianfeng wrote: > Make use of struct_size() helper instead of an open-coded calucation. > > Link: https://github.com/KSPP/linux/issues/160 > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> Reviewed-by: Kees Cook <keescook@chromium.org>
On Tue, Dec 14, 2021 at 07:48:54PM +0800, Xiu Jianfeng wrote: > Make use of struct_size() helper instead of an open-coded calucation. > > Link: https://github.com/KSPP/linux/issues/160 > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > --- > kernel/audit.c | 2 +- > kernel/audit_tree.c | 2 +- > kernel/auditfilter.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index d4084751cfe6..f33028578c60 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1446,7 +1446,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > if (err) > return err; > } > - sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL); > + sig_data = kmalloc(struct_size(sig_data, ctx, len), GFP_KERNEL); > if (!sig_data) { > if (audit_sig_sid) > security_release_secctx(ctx, len); > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index 72324afcffef..e7315d487163 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -94,7 +94,7 @@ static struct audit_tree *alloc_tree(const char *s) > { > struct audit_tree *tree; > > - tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); > + tree = kmalloc(struct_size(tree, pathname, strlen(s) + 1), GFP_KERNEL); > if (tree) { > refcount_set(&tree->count, 1); > tree->goner = 0; > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index 4173e771650c..19352820b274 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -637,7 +637,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule) > void *bufp; > int i; > > - data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL); > + data = kmalloc(struct_size(data, buf, krule->buflen), GFP_KERNEL); Why don't you also transform the zero-length array in struct audit_rule_data into a flexible-array member: 508 struct audit_rule_data { 509 __u32 flags; /* AUDIT_PER_{TASK,CALL}, AUDIT_PREPEND */ 510 __u32 action; /* AUDIT_NEVER, AUDIT_POSSIBLE, AUDIT_ALWAYS */ 511 __u32 field_count; 512 __u32 mask[AUDIT_BITMASK_SIZE]; /* syscall(s) affected */ 513 __u32 fields[AUDIT_MAX_FIELDS]; 514 __u32 values[AUDIT_MAX_FIELDS]; 515 __u32 fieldflags[AUDIT_MAX_FIELDS]; 516 __u32 buflen; /* total length of string fields */ 517 char buf[0]; /* string fields buffer */ 518 }; Thanks -- Gustavo > if (unlikely(!data)) > return NULL; > memset(data, 0, sizeof(*data)); > -- > 2.17.1 > > > >
On Tue, Dec 14, 2021 at 11:54:48AM -0600, Gustavo A. R. Silva wrote: > On Tue, Dec 14, 2021 at 07:48:54PM +0800, Xiu Jianfeng wrote: > > Make use of struct_size() helper instead of an open-coded calucation. > > > > Link: https://github.com/KSPP/linux/issues/160 > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > > --- > > kernel/audit.c | 2 +- > > kernel/audit_tree.c | 2 +- > > kernel/auditfilter.c | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index d4084751cfe6..f33028578c60 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c This could use struct_size(), too: 1461 audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0, 1462 sig_data, sizeof(*sig_data) + len); > > @@ -1446,7 +1446,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > > if (err) > > return err; > > } > > - sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL); > > + sig_data = kmalloc(struct_size(sig_data, ctx, len), GFP_KERNEL); > > if (!sig_data) { > > if (audit_sig_sid) > > security_release_secctx(ctx, len); > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index 72324afcffef..e7315d487163 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -94,7 +94,7 @@ static struct audit_tree *alloc_tree(const char *s) > > { > > struct audit_tree *tree; > > > > - tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); > > + tree = kmalloc(struct_size(tree, pathname, strlen(s) + 1), GFP_KERNEL); > > if (tree) { > > refcount_set(&tree->count, 1); > > tree->goner = 0; > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > index 4173e771650c..19352820b274 100644 > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c Also, in this same file the following piece of code could use struct_size(), too: 1093 skb = audit_make_reply(seq, AUDIT_LIST_RULES, 0, 1, 1094 data, 1095 sizeof(*data) + data->buflen); Thanks -- Gustavo > > @@ -637,7 +637,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule) > > void *bufp; > > int i; > > > > - data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL); > > + data = kmalloc(struct_size(data, buf, krule->buflen), GFP_KERNEL); > > Why don't you also transform the zero-length array in struct > audit_rule_data into a flexible-array member: > > 508 struct audit_rule_data { > 509 __u32 flags; /* AUDIT_PER_{TASK,CALL}, AUDIT_PREPEND */ > 510 __u32 action; /* AUDIT_NEVER, AUDIT_POSSIBLE, AUDIT_ALWAYS */ > 511 __u32 field_count; > 512 __u32 mask[AUDIT_BITMASK_SIZE]; /* syscall(s) affected */ > 513 __u32 fields[AUDIT_MAX_FIELDS]; > 514 __u32 values[AUDIT_MAX_FIELDS]; > 515 __u32 fieldflags[AUDIT_MAX_FIELDS]; > 516 __u32 buflen; /* total length of string fields */ > 517 char buf[0]; /* string fields buffer */ > 518 }; > > Thanks > -- > Gustavo > > > if (unlikely(!data)) > > return NULL; > > memset(data, 0, sizeof(*data)); > > -- > > 2.17.1 > > > > > > > >
On Tue, Dec 14, 2021 at 6:48 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote: > > Make use of struct_size() helper instead of an open-coded calucation. > > Link: https://github.com/KSPP/linux/issues/160 > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > --- > kernel/audit.c | 2 +- > kernel/audit_tree.c | 2 +- > kernel/auditfilter.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) Merged into audit/next, thanks!
在 2021/12/15 1:54, Gustavo A. R. Silva 写道: > On Tue, Dec 14, 2021 at 07:48:54PM +0800, Xiu Jianfeng wrote: >> Make use of struct_size() helper instead of an open-coded calucation. >> >> Link: https://github.com/KSPP/linux/issues/160 >> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> >> --- >> kernel/audit.c | 2 +- >> kernel/audit_tree.c | 2 +- >> kernel/auditfilter.c | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/audit.c b/kernel/audit.c >> index d4084751cfe6..f33028578c60 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -1446,7 +1446,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) >> if (err) >> return err; >> } >> - sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL); >> + sig_data = kmalloc(struct_size(sig_data, ctx, len), GFP_KERNEL); >> if (!sig_data) { >> if (audit_sig_sid) >> security_release_secctx(ctx, len); >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c >> index 72324afcffef..e7315d487163 100644 >> --- a/kernel/audit_tree.c >> +++ b/kernel/audit_tree.c >> @@ -94,7 +94,7 @@ static struct audit_tree *alloc_tree(const char *s) >> { >> struct audit_tree *tree; >> >> - tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); >> + tree = kmalloc(struct_size(tree, pathname, strlen(s) + 1), GFP_KERNEL); >> if (tree) { >> refcount_set(&tree->count, 1); >> tree->goner = 0; >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> index 4173e771650c..19352820b274 100644 >> --- a/kernel/auditfilter.c >> +++ b/kernel/auditfilter.c >> @@ -637,7 +637,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule) >> void *bufp; >> int i; >> >> - data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL); >> + data = kmalloc(struct_size(data, buf, krule->buflen), GFP_KERNEL); > Why don't you also transform the zero-length array in struct > audit_rule_data into a flexible-array member: > > 508 struct audit_rule_data { > 509 __u32 flags; /* AUDIT_PER_{TASK,CALL}, AUDIT_PREPEND */ > 510 __u32 action; /* AUDIT_NEVER, AUDIT_POSSIBLE, AUDIT_ALWAYS */ > 511 __u32 field_count; > 512 __u32 mask[AUDIT_BITMASK_SIZE]; /* syscall(s) affected */ > 513 __u32 fields[AUDIT_MAX_FIELDS]; > 514 __u32 values[AUDIT_MAX_FIELDS]; > 515 __u32 fieldflags[AUDIT_MAX_FIELDS]; > 516 __u32 buflen; /* total length of string fields */ > 517 char buf[0]; /* string fields buffer */ > 518 }; > > Thanks > -- > Gustavo thank you, I will add it in the v2 patch. >> if (unlikely(!data)) >> return NULL; >> memset(data, 0, sizeof(*data)); >> -- >> 2.17.1 >> >> >> >> > .
在 2021/12/15 2:10, Gustavo A. R. Silva 写道: > On Tue, Dec 14, 2021 at 11:54:48AM -0600, Gustavo A. R. Silva wrote: >> On Tue, Dec 14, 2021 at 07:48:54PM +0800, Xiu Jianfeng wrote: >>> Make use of struct_size() helper instead of an open-coded calucation. >>> >>> Link: https://github.com/KSPP/linux/issues/160 >>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> >>> --- >>> kernel/audit.c | 2 +- >>> kernel/audit_tree.c | 2 +- >>> kernel/auditfilter.c | 2 +- >>> 3 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/kernel/audit.c b/kernel/audit.c >>> index d4084751cfe6..f33028578c60 100644 >>> --- a/kernel/audit.c >>> +++ b/kernel/audit.c > This could use struct_size(), too: > > 1461 audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0, > 1462 sig_data, sizeof(*sig_data) + len); > >>> @@ -1446,7 +1446,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) >>> if (err) >>> return err; >>> } >>> - sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL); >>> + sig_data = kmalloc(struct_size(sig_data, ctx, len), GFP_KERNEL); >>> if (!sig_data) { >>> if (audit_sig_sid) >>> security_release_secctx(ctx, len); >>> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c >>> index 72324afcffef..e7315d487163 100644 >>> --- a/kernel/audit_tree.c >>> +++ b/kernel/audit_tree.c >>> @@ -94,7 +94,7 @@ static struct audit_tree *alloc_tree(const char *s) >>> { >>> struct audit_tree *tree; >>> >>> - tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); >>> + tree = kmalloc(struct_size(tree, pathname, strlen(s) + 1), GFP_KERNEL); >>> if (tree) { >>> refcount_set(&tree->count, 1); >>> tree->goner = 0; >>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >>> index 4173e771650c..19352820b274 100644 >>> --- a/kernel/auditfilter.c >>> +++ b/kernel/auditfilter.c > Also, in this same file the following piece of code could use > struct_size(), too: > > 1093 skb = audit_make_reply(seq, AUDIT_LIST_RULES, 0, 1, > 1094 data, > 1095 sizeof(*data) + data->buflen); > > Thanks > -- > Gustavo thanks, I missed these two places, I will add them in the v2 patch. >>> @@ -637,7 +637,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule) >>> void *bufp; >>> int i; >>> >>> - data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL); >>> + data = kmalloc(struct_size(data, buf, krule->buflen), GFP_KERNEL); >> Why don't you also transform the zero-length array in struct >> audit_rule_data into a flexible-array member: >> >> 508 struct audit_rule_data { >> 509 __u32 flags; /* AUDIT_PER_{TASK,CALL}, AUDIT_PREPEND */ >> 510 __u32 action; /* AUDIT_NEVER, AUDIT_POSSIBLE, AUDIT_ALWAYS */ >> 511 __u32 field_count; >> 512 __u32 mask[AUDIT_BITMASK_SIZE]; /* syscall(s) affected */ >> 513 __u32 fields[AUDIT_MAX_FIELDS]; >> 514 __u32 values[AUDIT_MAX_FIELDS]; >> 515 __u32 fieldflags[AUDIT_MAX_FIELDS]; >> 516 __u32 buflen; /* total length of string fields */ >> 517 char buf[0]; /* string fields buffer */ >> 518 }; >> >> Thanks >> -- >> Gustavo >> >>> if (unlikely(!data)) >>> return NULL; >>> memset(data, 0, sizeof(*data)); >>> -- >>> 2.17.1 >>> >>> >>> >>> > .
在 2021/12/15 6:47, Paul Moore 写道: > On Tue, Dec 14, 2021 at 6:48 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote: >> Make use of struct_size() helper instead of an open-coded calucation. >> >> Link: https://github.com/KSPP/linux/issues/160 >> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> >> --- >> kernel/audit.c | 2 +- >> kernel/audit_tree.c | 2 +- >> kernel/auditfilter.c | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) > Merged into audit/next, thanks! thank you, but I missed two places, do you mind if I send a v2 patch?
diff --git a/kernel/audit.c b/kernel/audit.c index d4084751cfe6..f33028578c60 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1446,7 +1446,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) if (err) return err; } - sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL); + sig_data = kmalloc(struct_size(sig_data, ctx, len), GFP_KERNEL); if (!sig_data) { if (audit_sig_sid) security_release_secctx(ctx, len); diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 72324afcffef..e7315d487163 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -94,7 +94,7 @@ static struct audit_tree *alloc_tree(const char *s) { struct audit_tree *tree; - tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); + tree = kmalloc(struct_size(tree, pathname, strlen(s) + 1), GFP_KERNEL); if (tree) { refcount_set(&tree->count, 1); tree->goner = 0; diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 4173e771650c..19352820b274 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -637,7 +637,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule) void *bufp; int i; - data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL); + data = kmalloc(struct_size(data, buf, krule->buflen), GFP_KERNEL); if (unlikely(!data)) return NULL; memset(data, 0, sizeof(*data));
Make use of struct_size() helper instead of an open-coded calucation. Link: https://github.com/KSPP/linux/issues/160 Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> --- kernel/audit.c | 2 +- kernel/audit_tree.c | 2 +- kernel/auditfilter.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)