Re: [PATCH] printk: Remove possible overflow in user read buffer

[Petr Mladek <pmladek@suse.cz>]

On Wed 2015-05-20 12:59:48, Marcin Niesluchowski wrote:
> Reading message with dict may cause user buffer overflow due to
> no limits of written dict and hardcoded user read buffer size.
> As limits of dict are not clear, it may be possible in extreme
> use case to trigger it (e.g. by driver passing some dict parameters
> from userland or other module logging large key-value data).
> 
> Truncate dict read by user when its size would cause user read
> buffer to overflow.
> 
> Bug was found during work on extension of kmsg enabling writing
> dict from userspace.

By coincidence, Tejun has already provided slightly different patch
for this problem, see https://lkml.org/lkml/2015/5/14/494

AFAIK, Tejun's patch already is in the -mm tree and thus on the way to
get merged.

I would prefer Tejun's solution.

Best Regards,
Petr

> 
> Signed-off-by: Marcin Niesluchowski <m.niesluchow@samsung.com>
> ---
>  kernel/printk/printk.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index c099b08..b61602d 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -505,6 +505,7 @@ int check_syslog_permissions(int type, bool from_file)
>  	return security_syslog(type);
>  }
>  
> +#define USER_READ_LOG_BUF_LEN	8192
>  
>  /* /dev/kmsg - userspace message inject/listen interface */
>  struct devkmsg_user {
> @@ -512,7 +513,7 @@ struct devkmsg_user {
>  	u32 idx;
>  	enum log_flags prev;
>  	struct mutex lock;
> -	char buf[8192];
> +	char buf[USER_READ_LOG_BUF_LEN];
>  };
>  
>  static ssize_t devkmsg_write(struct kiocb *iocb, struct iov_iter *from)
> @@ -648,21 +649,29 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>  			unsigned char c = log_dict(msg)[i];
>  
>  			if (line) {
> +				if (len >= USER_READ_LOG_BUF_LEN-1)
> +					break;
>  				user->buf[len++] = ' ';
>  				line = false;
>  			}
>  
>  			if (c == '\0') {
> +				if (len >= USER_READ_LOG_BUF_LEN-1)
> +					break;
>  				user->buf[len++] = '\n';
>  				line = true;
>  				continue;
>  			}
>  
>  			if (c < ' ' || c >= 127 || c == '\\') {
> +				if (len >= USER_READ_LOG_BUF_LEN-4)
> +					break;
>  				len += sprintf(user->buf + len, "\\x%02x", c);
>  				continue;
>  			}
>  
> +			if (len >= USER_READ_LOG_BUF_LEN-1)
> +				break;
>  			user->buf[len++] = c;
>  		}
>  		user->buf[len++] = '\n';
> -- 
> 1.9.1
>