On 9/3/2019 11:01 AM, Denis Efremov wrote: > request_buffer is required to describe an access type in a string for > the audit. The problem here is that the string is saved on the stack > and then passed by reference to the next function in request field of > the smack_audit_data structure. Referencing variables on a stack > and saving them in external data structures is usually considered > as bad and error-prone practice.
You're adding space to the smack_audit_data structure on the off chance that the stack might disappear out from under something this function is calling. If you trace the code path, you'll find that doesn't happen. I can't say that I see any real value to this change. > Thus, this commit simply moves > the request_buffer from the stack to the stack_audit_data structure > and removes the necessity of stack referencing. strcat calls are > replaced with strlcat calls - a safer analog for strings concatenation > with bounds checking. Changing strcat to strlcat (or any variant, for that matter) when the source is a string constant and the destination size is known is completely pointless. > > Signed-off-by: Denis Efremov <[email protected]> I appreciate the intention, but I don't see any real value here. I won't be taking this. > --- > security/smack/smack.h | 6 +++++- > security/smack/smack_access.c | 12 +++--------- > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/security/smack/smack.h b/security/smack/smack.h > index 62529f382942..9eeefb865dfd 100644 > --- a/security/smack/smack.h > +++ b/security/smack/smack.h > @@ -278,7 +278,11 @@ struct smack_audit_data { > const char *function; > char *subject; > char *object; > - char *request; > +#ifdef CONFIG_SECURITY_SMACK_BRINGUP > + char request[SMK_NUM_ACCESS_TYPE + 5]; > +#else > + char request[SMK_NUM_ACCESS_TYPE + 1]; > +#endif > int result; > }; > > diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c > index f1c93a7be9ec..99e58d4a9980 100644 > --- a/security/smack/smack_access.c > +++ b/security/smack/smack_access.c > @@ -340,11 +340,6 @@ static void smack_log_callback(struct audit_buffer *ab, > void *a) > void smack_log(char *subject_label, char *object_label, int request, > int result, struct smk_audit_info *ad) > { > -#ifdef CONFIG_SECURITY_SMACK_BRINGUP > - char request_buffer[SMK_NUM_ACCESS_TYPE + 5]; > -#else > - char request_buffer[SMK_NUM_ACCESS_TYPE + 1]; > -#endif > struct smack_audit_data *sad; > struct common_audit_data *a = &ad->a; > > @@ -360,7 +355,7 @@ void smack_log(char *subject_label, char *object_label, > int request, > sad->function = "unknown"; > > /* end preparing the audit data */ > - smack_str_from_perm(request_buffer, request); > + smack_str_from_perm(sad->request, request); > sad->subject = subject_label; > sad->object = object_label; > #ifdef CONFIG_SECURITY_SMACK_BRINGUP > @@ -371,14 +366,13 @@ void smack_log(char *subject_label, char *object_label, > int request, > * the logging policy says to do so. > */ > if (result == SMACK_UNCONFINED_SUBJECT) > - strcat(request_buffer, "(US)"); > + strlcat(sad->request, "(US)", sizeof(sad->request)); Have you ever heard of a C compiler that would not correctly terminate a constant string? I've been using them for over 40 years and have never seen a case where this was a problem. > else if (result == SMACK_UNCONFINED_OBJECT) > - strcat(request_buffer, "(UO)"); > + strlcat(sad->request, "(UO)", sizeof(sad->request)); > > if (result > 0) > result = 0; > #endif > - sad->request = request_buffer; > sad->result = result; > > common_lsm_audit(a, smack_log_callback, NULL);
