[PATCH] lockdown: avoid extra call to strlen() in lockdown_read()

[PATCH] lockdown: avoid extra call to strlen() in lockdown_read()

[Dmitry Antipov]

Since s*printf() family of functions returns the number of characters
emitted, avoid redundant call to strlen() in lockdown_read() and prefer
snprintf() over sprintf() for an extra protection against buffer overflow.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 security/lockdown/lockdown.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index cf83afa1d879..10537d7c4437 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -106,9 +106,13 @@ static ssize_t lockdown_read(struct file *filp, char __user *buf, size_t count,
 			const char *label = lockdown_reasons[level];
 
 			if (kernel_locked_down == level)
-				offset += sprintf(temp+offset, "[%s] ", label);
+				offset += snprintf(temp + offset,
+						   sizeof(temp) - offset,
+						   "[%s] ", label);
 			else
-				offset += sprintf(temp+offset, "%s ", label);
+				offset += snprintf(temp + offset,
+						   sizeof(temp) - offset,
+						   "%s ", label);
 		}
 	}
 
@@ -116,7 +120,7 @@ static ssize_t lockdown_read(struct file *filp, char __user *buf, size_t count,
 	if (offset > 0)
 		temp[offset-1] = '\n';
 
-	return simple_read_from_buffer(buf, count, ppos, temp, strlen(temp));
+	return simple_read_from_buffer(buf, count, ppos, temp, offset);
 }
 
 static ssize_t lockdown_write(struct file *file, const char __user *buf,
-- 
2.50.1

Re: [PATCH] lockdown: avoid extra call to strlen() in lockdown_read()

[Xiujianfeng]

Hi Dmitry,

> Since s*printf() family of functions returns the number of characters emitted,
> avoid redundant call to strlen() in lockdown_read() and prefer
> snprintf() over sprintf() for an extra protection against buffer overflow.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  security/lockdown/lockdown.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index cf83afa1d879..10537d7c4437 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -106,9 +106,13 @@ static ssize_t lockdown_read(struct file *filp, char
> __user *buf, size_t count,
>  			const char *label = lockdown_reasons[level];
> 
>  			if (kernel_locked_down == level)
> -				offset += sprintf(temp+offset, "[%s] ", label);
> +				offset += snprintf(temp + offset,
> +						   sizeof(temp) - offset,
> +						   "[%s] ", label);
>  			else
> -				offset += sprintf(temp+offset, "%s ", label);
> +				offset += snprintf(temp + offset,
> +						   sizeof(temp) - offset,
> +						   "%s ", label);
>  		}
>  	}
> 
> @@ -116,7 +120,7 @@ static ssize_t lockdown_read(struct file *filp, char
> __user *buf, size_t count,
>  	if (offset > 0)
>  		temp[offset-1] = '\n';
> 
> -	return simple_read_from_buffer(buf, count, ppos, temp, strlen(temp));
> +	return simple_read_from_buffer(buf, count, ppos, temp, offset);

Thanks for your patch.

Since the current `lockdown_levels` array is static and has only three members,
and the total number of characters of these three reasons is far from exceeding 80,
there seems to be no risk of buffer overflow for now.

About the change to strlen, I think lockdown_read() is not on the hot path, the impact
is minimal.

So I prefer to leave them as is, thanks.

Best regards,

Xiu

>  }
> 
>  static ssize_t lockdown_write(struct file *file, const char __user *buf,
> --
> 2.50.1