| Message ID | 20230817210327.never.598-kees@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | integrity: Annotate struct ima_rule_opt_list with __counted_by | expand |
On 8/17/23 15:03, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct ima_rule_opt_list. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Mimi Zohar <zohar@linux.ibm.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: James Morris <jmorris@namei.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: linux-integrity@vger.kernel.org > Cc: linux-security-module@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks -- Gustavo > --- > security/integrity/ima/ima_policy.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 69452b79686b..f69062617754 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -68,7 +68,7 @@ enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY }; > > struct ima_rule_opt_list { > size_t count; > - char *items[]; > + char *items[] __counted_by(count); > }; > > /* > @@ -342,6 +342,7 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) > kfree(src_copy); > return ERR_PTR(-ENOMEM); > } > + opt_list->count = count; > > /* > * strsep() has already replaced all instances of '|' with '\0', > @@ -357,7 +358,6 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) > opt_list->items[i] = cur; > cur = strchr(cur, '\0') + 1; > } > - opt_list->count = count; > > return opt_list; > }
On Fri Aug 18, 2023 at 12:03 AM EEST, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct ima_rule_opt_list. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Mimi Zohar <zohar@linux.ibm.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: James Morris <jmorris@namei.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: linux-integrity@vger.kernel.org > Cc: linux-security-module@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > security/integrity/ima/ima_policy.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 69452b79686b..f69062617754 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -68,7 +68,7 @@ enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY }; > > struct ima_rule_opt_list { > size_t count; > - char *items[]; > + char *items[] __counted_by(count); > }; > > /* > @@ -342,6 +342,7 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) > kfree(src_copy); > return ERR_PTR(-ENOMEM); > } > + opt_list->count = count; > > /* > * strsep() has already replaced all instances of '|' with '\0', > @@ -357,7 +358,6 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) > opt_list->items[i] = cur; > cur = strchr(cur, '\0') + 1; > } > - opt_list->count = count; > > return opt_list; > } > -- > 2.34.1 Acked-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
On Thu, 2023-08-17 at 14:03 -0700, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct ima_rule_opt_list. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Mimi Zohar <zohar@linux.ibm.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: James Morris <jmorris@namei.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: linux-integrity@vger.kernel.org > Cc: linux-security-module@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Acked-by: Mimi Zohar <zohar@linux.ibm.com>
On Thu, 17 Aug 2023 14:03:28 -0700, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct ima_rule_opt_list. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [...] Applied to for-next/hardening, thanks! [1/1] integrity: Annotate struct ima_rule_opt_list with __counted_by https://git.kernel.org/kees/c/a4b35d4d05b9 Take care,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 69452b79686b..f69062617754 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -68,7 +68,7 @@ enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY }; struct ima_rule_opt_list { size_t count; - char *items[]; + char *items[] __counted_by(count); }; /* @@ -342,6 +342,7 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) kfree(src_copy); return ERR_PTR(-ENOMEM); } + opt_list->count = count; /* * strsep() has already replaced all instances of '|' with '\0', @@ -357,7 +358,6 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) opt_list->items[i] = cur; cur = strchr(cur, '\0') + 1; } - opt_list->count = count; return opt_list; }
Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by for struct ima_rule_opt_list. Additionally, since the element count member must be set before accessing the annotated flexible array member, move its initialization earlier. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Mimi Zohar <zohar@linux.ibm.com> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> Cc: Paul Moore <paul@paul-moore.com> Cc: James Morris <jmorris@namei.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: linux-integrity@vger.kernel.org Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- security/integrity/ima/ima_policy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)