linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Smack: Move request_buffer from stack to smack_audit_data
@ 2019-09-03 18:01 Denis Efremov
  2019-09-03 20:56 ` Casey Schaufler
  0 siblings, 1 reply; 2+ messages in thread
From: Denis Efremov @ 2019-09-03 18:01 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Denis Efremov, Serge E. Hallyn, linux-security-module, linux-kernel

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. 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.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 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));
 	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);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] Smack: Move request_buffer from stack to smack_audit_data
  2019-09-03 18:01 [PATCH] Smack: Move request_buffer from stack to smack_audit_data Denis Efremov
@ 2019-09-03 20:56 ` Casey Schaufler
  0 siblings, 0 replies; 2+ messages in thread
From: Casey Schaufler @ 2019-09-03 20:56 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Serge E. Hallyn, linux-security-module, linux-kernel, casey

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 <efremov@linux.com>

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);


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2019-09-03 20:56 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-03 18:01 [PATCH] Smack: Move request_buffer from stack to smack_audit_data Denis Efremov
2019-09-03 20:56 ` Casey Schaufler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).