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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150520121044.GA2728@pathway.suse.cz>
Date:	Wed, 20 May 2015 14:10:44 +0200
From:	Petr Mladek <pmladek@...e.cz>
To:	Marcin Niesluchowski <m.niesluchow@...sung.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, Jan Kara <jack@...e.cz>,
	"Steven Rostedt (Red Hat)" <rostedt@...dmis.org>,
	Alex Elder <elder@...aro.org>,
	"Luis R. Rodriguez" <mcgrof@...e.com>,
	Peter Hurley <peter@...leysoftware.com>,
	Joe Perches <joe@...ches.com>, Kay Sievers <kay@...y.org>,
	linux-kernel@...r.kernel.org,
	Łukasz Stelmach <l.stelmach@...sung.com>,
	Karol Lewandowski <k.lewandowsk@...sung.com>,
	Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH] printk: Remove possible overflow in user read buffer

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@...sung.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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/