2015-05-20 11:00:32

by Marcin Niesluchowski

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

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.

Signed-off-by: Marcin Niesluchowski <[email protected]>
---
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

2015-05-20 12:10:50

by Petr Mladek

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 <[email protected]>
> ---
> 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
>