| Message ID | 20180706201809.105152-1-jannh@google.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] net/mlx5: fix uaccess beyond "count" in debugfs read/write handlers | expand |
On Fri, Jul 06, 2018 at 10:18:09PM +0200, Jann Horn wrote: > In general, accessing userspace memory beyond the length of the supplied > buffer in VFS read/write handlers can lead to both kernel memory corruption > (via kernel_read()/kernel_write(), which can e.g. be triggered via > sys_splice()) and privilege escalation inside userspace. > > In this case, the affected files are in debugfs (and should therefore only > be accessible to root) and check that *pos is zero (which prevents the > sys_splice() trick). Therefore, this is not a security fix, but rather a > small cleanup. > > For the read handlers, fix it by using simple_read_from_buffer() instead of > custom logic. > For the write handler, add a check. > > changed in v2: > - also fix dbg_write() > Thanks Jann, Next time, please don't put changelog in commit message. Saeed, are you taking it to mlx5-next? It is cleanup and better to be sent to -next. > Fixes: e126ba97dba9 ("mlx5: Add driver for Mellanox Connect-IB adapters") > Signed-off-by: Jann Horn <jannh@google.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 28 +++++-------------- > .../net/ethernet/mellanox/mlx5/core/debugfs.c | 22 ++------------- > 2 files changed, 9 insertions(+), 41 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Fri, 2018-07-06 at 22:18 +0200, Jann Horn wrote: > In general, accessing userspace memory beyond the length of the > supplied > buffer in VFS read/write handlers can lead to both kernel memory > corruption > (via kernel_read()/kernel_write(), which can e.g. be triggered via > sys_splice()) and privilege escalation inside userspace. > > In this case, the affected files are in debugfs (and should therefore > only > be accessible to root) and check that *pos is zero (which prevents > the > sys_splice() trick). Therefore, this is not a security fix, but > rather a > small cleanup. > > For the read handlers, fix it by using simple_read_from_buffer() > instead of > custom logic. > For the write handler, add a check. > > changed in v2: > - also fix dbg_write() > > Fixes: e126ba97dba9 ("mlx5: Add driver for Mellanox Connect-IB > adapters") > Signed-off-by: Jann Horn <jannh@google.com> > Applied to mlx5-next, Thanks!!
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c index 384c1fa49081..92578074145d 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c @@ -1022,7 +1022,10 @@ static ssize_t dbg_write(struct file *filp, const char __user *buf, if (!dbg->in_msg || !dbg->out_msg) return -ENOMEM; - if (copy_from_user(lbuf, buf, sizeof(lbuf))) + if (count < sizeof(lbuf) - 1) + return -EINVAL; + + if (copy_from_user(lbuf, buf, sizeof(lbuf) - 1)) return -EFAULT; lbuf[sizeof(lbuf) - 1] = 0; @@ -1226,21 +1229,12 @@ static ssize_t data_read(struct file *filp, char __user *buf, size_t count, { struct mlx5_core_dev *dev = filp->private_data; struct mlx5_cmd_debug *dbg = &dev->cmd.dbg; - int copy; - - if (*pos) - return 0; if (!dbg->out_msg) return -ENOMEM; - copy = min_t(int, count, dbg->outlen); - if (copy_to_user(buf, dbg->out_msg, copy)) - return -EFAULT; - - *pos += copy; - - return copy; + return simple_read_from_buffer(buf, count, pos, dbg->out_msg, + dbg->outlen); } static const struct file_operations dfops = { @@ -1258,19 +1252,11 @@ static ssize_t outlen_read(struct file *filp, char __user *buf, size_t count, char outlen[8]; int err; - if (*pos) - return 0; - err = snprintf(outlen, sizeof(outlen), "%d", dbg->outlen); if (err < 0) return err; - if (copy_to_user(buf, &outlen, err)) - return -EFAULT; - - *pos += err; - - return err; + return simple_read_from_buffer(buf, count, pos, outlen, err); } static ssize_t outlen_write(struct file *filp, const char __user *buf, diff --git a/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c b/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c index 413080a312a7..90fabd612b6c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c @@ -150,22 +150,13 @@ static ssize_t average_read(struct file *filp, char __user *buf, size_t count, int ret; char tbuf[22]; - if (*pos) - return 0; - stats = filp->private_data; spin_lock_irq(&stats->lock); if (stats->n) field = div64_u64(stats->sum, stats->n); spin_unlock_irq(&stats->lock); ret = snprintf(tbuf, sizeof(tbuf), "%llu\n", field); - if (ret > 0) { - if (copy_to_user(buf, tbuf, ret)) - return -EFAULT; - } - - *pos += ret; - return ret; + return simple_read_from_buffer(buf, count, pos, tbuf, ret); } static ssize_t average_write(struct file *filp, const char __user *buf, @@ -442,9 +433,6 @@ static ssize_t dbg_read(struct file *filp, char __user *buf, size_t count, u64 field; int ret; - if (*pos) - return 0; - desc = filp->private_data; d = (void *)(desc - desc->i) - sizeof(*d); switch (d->type) { @@ -470,13 +458,7 @@ static ssize_t dbg_read(struct file *filp, char __user *buf, size_t count, else ret = snprintf(tbuf, sizeof(tbuf), "0x%llx\n", field); - if (ret > 0) { - if (copy_to_user(buf, tbuf, ret)) - return -EFAULT; - } - - *pos += ret; - return ret; + return simple_read_from_buffer(buf, count, pos, tbuf, ret); } static const struct file_operations fops = {
In general, accessing userspace memory beyond the length of the supplied buffer in VFS read/write handlers can lead to both kernel memory corruption (via kernel_read()/kernel_write(), which can e.g. be triggered via sys_splice()) and privilege escalation inside userspace. In this case, the affected files are in debugfs (and should therefore only be accessible to root) and check that *pos is zero (which prevents the sys_splice() trick). Therefore, this is not a security fix, but rather a small cleanup. For the read handlers, fix it by using simple_read_from_buffer() instead of custom logic. For the write handler, add a check. changed in v2: - also fix dbg_write() Fixes: e126ba97dba9 ("mlx5: Add driver for Mellanox Connect-IB adapters") Signed-off-by: Jann Horn <jannh@google.com> --- drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 28 +++++-------------- .../net/ethernet/mellanox/mlx5/core/debugfs.c | 22 ++------------- 2 files changed, 9 insertions(+), 41 deletions(-)