From: Miklos Szeredi <[email protected]>
Some callers (aio_run_iocb, vmsplice_to_user) forget to free the iov on
error. This seems to be a recurring problem, with most callers being buggy
initially.
So instead of fixing the callers, fix the semantics: free the allocated iov
on error, so callers don't have to.
We could return either fast_pointer or NULL for the error case. I've opted
for NULL.
Found by Coverity Scan.
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: [email protected]
---
fs/compat.c | 19 +++++++++++++------
fs/read_write.c | 13 ++++++++++---
2 files changed, 23 insertions(+), 9 deletions(-)
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -689,7 +689,7 @@ ssize_t rw_copy_check_uvector(int type,
}
if (copy_from_user(iov, uvector, nr_segs*sizeof(*uvector))) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
/*
@@ -710,12 +710,12 @@ ssize_t rw_copy_check_uvector(int type,
* it's about to overflow ssize_t */
if (len < 0) {
ret = -EINVAL;
- goto out;
+ goto out_free;
}
if (type >= 0
&& unlikely(!access_ok(vrfy_dir(type), buf, len))) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
if (len > MAX_RW_COUNT - ret) {
len = MAX_RW_COUNT - ret;
@@ -726,6 +726,13 @@ ssize_t rw_copy_check_uvector(int type,
out:
*ret_pointer = iov;
return ret;
+
+out_free:
+ if (iov != fast_pointer) {
+ kfree(iov);
+ iov = NULL;
+ }
+ goto out;
}
static ssize_t do_readv_writev(int type, struct file *file,
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -549,7 +549,7 @@ ssize_t compat_rw_copy_check_uvector(int
struct iovec **ret_pointer)
{
compat_ssize_t tot_len;
- struct iovec *iov = *ret_pointer = fast_pointer;
+ struct iovec *iov = fast_pointer;
ssize_t ret = 0;
int seg;
@@ -570,11 +570,10 @@ ssize_t compat_rw_copy_check_uvector(int
if (iov == NULL)
goto out;
}
- *ret_pointer = iov;
ret = -EFAULT;
if (!access_ok(VERIFY_READ, uvector, nr_segs*sizeof(*uvector)))
- goto out;
+ goto out_free;
/*
* Single unix specification:
@@ -593,14 +592,14 @@ ssize_t compat_rw_copy_check_uvector(int
if (__get_user(len, &uvector->iov_len) ||
__get_user(buf, &uvector->iov_base)) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
if (len < 0) /* size_t not fitting in compat_ssize_t .. */
- goto out;
+ goto out_free;
if (type >= 0 &&
!access_ok(vrfy_dir(type), compat_ptr(buf), len)) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
if (len > MAX_RW_COUNT - tot_len)
len = MAX_RW_COUNT - tot_len;
@@ -613,7 +612,15 @@ ssize_t compat_rw_copy_check_uvector(int
ret = tot_len;
out:
+ *ret_pointer = iov;
return ret;
+
+out_free:
+ if (iov != fast_pointer) {
+ kfree(iov);
+ iov = NULL;
+ }
+ goto out;
}
static inline long
On Tue, Apr 15, 2014 at 04:57:49PM +0200, Miklos Szeredi wrote:
> Some callers (aio_run_iocb, vmsplice_to_user) forget to free the iov on
> error. This seems to be a recurring problem, with most callers being buggy
> initially.
Your patch looks a lot more complete than the quick hack I did a few
days ago when coverity first started nagging about this, but in testing
I've found that something really ugly starts showing up when you patch this
The symptoms vary, but always are some kind of slab corruption.
Here's the last example:
=============================================================================
BUG kmalloc-256 (Not tainted): Invalid object pointer 0xffff8802407adc60
-----------------------------------------------------------------------------
Disabling lock debugging due to kernel taint
INFO: Slab 0xffffea000901eb00 objects=28 used=22 fp=0xffff8802407ad6d0 flags=0x20000000004081
CPU: 1 PID: 1185 Comm: trinity-c1 Tainted: G B 3.15.0-rc1+ #191
ffff880243c073c0 00000000f952f249 ffff8800a1a2bc10 ffffffffbd74686d
ffffea000901eb00 ffff8800a1a2bce8 ffffffffbd1b0cd4 ffffffff00000020
ffff8800a1a2bcf8 ffff8800a1a2bca8 61766e4943c00a18 656a626f2064696c
Call Trace:
[<ffffffffbd74686d>] dump_stack+0x4e/0x7a
[<ffffffffbd1b0cd4>] slab_err+0xb4/0xe0
[<ffffffffbd0bf3ae>] ? put_lock_stats.isra.23+0xe/0x30
[<ffffffffbd1b0da6>] ? slab_pad_check.part.44+0xa6/0x170
[<ffffffffbd744e7f>] free_debug_processing+0x88/0x22a
[<ffffffffbd1c7041>] ? compat_do_readv_writev+0xe1/0x250
[<ffffffffbd74506d>] __slab_free+0x4c/0x2c3
[<ffffffffbd1c6679>] ? do_sync_readv_writev+0x59/0xa0
[<ffffffffbd1b2614>] kfree+0x214/0x220
[<ffffffffbd1c7041>] ? compat_do_readv_writev+0xe1/0x250
[<ffffffffbd1c7041>] compat_do_readv_writev+0xe1/0x250
[<ffffffffbd0bf716>] ? lock_release_holdtime.part.24+0xe6/0x160
[<ffffffffbd0a3ccd>] ? get_parent_ip+0xd/0x50
[<ffffffffbd75642b>] ? preempt_count_sub+0x6b/0xf0
[<ffffffffbd751a01>] ? _raw_spin_unlock+0x31/0x50
[<ffffffffbd349883>] ? __this_cpu_preempt_check+0x13/0x20
[<ffffffffbd1c730a>] compat_writev+0x3a/0x80
[<ffffffffbd1c85d8>] compat_SyS_writev+0x58/0xd0
[<ffffffffbd75c6a9>] ia32_do_call+0x13/0x13
FIX kmalloc-256: Object at 0xffff8802407adc60 not freed
I also had an incomplete trace that showed vmsplice causing a bug in mm/slub.c:3396
on an earlier run.
The crash happens very quickly (within a few seconds of running trinity) for me.
Dave
On Wed, Apr 16, 2014 at 02:04:22PM -0400, Dave Jones wrote:
> On Tue, Apr 15, 2014 at 04:57:49PM +0200, Miklos Szeredi wrote:
>
> > Some callers (aio_run_iocb, vmsplice_to_user) forget to free the iov on
> > error. This seems to be a recurring problem, with most callers being buggy
> > initially.
>
> Your patch looks a lot more complete than the quick hack I did a few
> days ago when coverity first started nagging about this, but in testing
> I've found that something really ugly starts showing up when you patch this
>
> The symptoms vary, but always are some kind of slab corruption.
> Here's the last example:
>
> =============================================================================
> BUG kmalloc-256 (Not tainted): Invalid object pointer 0xffff8802407adc60
> -----------------------------------------------------------------------------
>
> Disabling lock debugging due to kernel taint
> INFO: Slab 0xffffea000901eb00 objects=28 used=22 fp=0xffff8802407ad6d0 flags=0x20000000004081
> CPU: 1 PID: 1185 Comm: trinity-c1 Tainted: G B 3.15.0-rc1+ #191
> ffff880243c073c0 00000000f952f249 ffff8800a1a2bc10 ffffffffbd74686d
> ffffea000901eb00 ffff8800a1a2bce8 ffffffffbd1b0cd4 ffffffff00000020
> ffff8800a1a2bcf8 ffff8800a1a2bca8 61766e4943c00a18 656a626f2064696c
> Call Trace:
> [<ffffffffbd74686d>] dump_stack+0x4e/0x7a
> [<ffffffffbd1b0cd4>] slab_err+0xb4/0xe0
> [<ffffffffbd0bf3ae>] ? put_lock_stats.isra.23+0xe/0x30
> [<ffffffffbd1b0da6>] ? slab_pad_check.part.44+0xa6/0x170
> [<ffffffffbd744e7f>] free_debug_processing+0x88/0x22a
> [<ffffffffbd1c7041>] ? compat_do_readv_writev+0xe1/0x250
> [<ffffffffbd74506d>] __slab_free+0x4c/0x2c3
> [<ffffffffbd1c6679>] ? do_sync_readv_writev+0x59/0xa0
> [<ffffffffbd1b2614>] kfree+0x214/0x220
> [<ffffffffbd1c7041>] ? compat_do_readv_writev+0xe1/0x250
> [<ffffffffbd1c7041>] compat_do_readv_writev+0xe1/0x250
> [<ffffffffbd0bf716>] ? lock_release_holdtime.part.24+0xe6/0x160
> [<ffffffffbd0a3ccd>] ? get_parent_ip+0xd/0x50
> [<ffffffffbd75642b>] ? preempt_count_sub+0x6b/0xf0
> [<ffffffffbd751a01>] ? _raw_spin_unlock+0x31/0x50
> [<ffffffffbd349883>] ? __this_cpu_preempt_check+0x13/0x20
> [<ffffffffbd1c730a>] compat_writev+0x3a/0x80
> [<ffffffffbd1c85d8>] compat_SyS_writev+0x58/0xd0
> [<ffffffffbd75c6a9>] ia32_do_call+0x13/0x13
> FIX kmalloc-256: Object at 0xffff8802407adc60 not freed
>
>
> I also had an incomplete trace that showed vmsplice causing a bug in mm/slub.c:3396
> on an earlier run.
>
> The crash happens very quickly (within a few seconds of running trinity) for me.
Al, Miklos ?
Dave
On Mon, Apr 21, 2014 at 5:50 PM, Dave Jones <[email protected]> wrote:
> On Wed, Apr 16, 2014 at 02:04:22PM -0400, Dave Jones wrote:
> > On Tue, Apr 15, 2014 at 04:57:49PM +0200, Miklos Szeredi wrote:
> >
> > > Some callers (aio_run_iocb, vmsplice_to_user) forget to free the iov on
> > > error. This seems to be a recurring problem, with most callers being buggy
> > > initially.
> >
> > Your patch looks a lot more complete than the quick hack I did a few
> > days ago when coverity first started nagging about this, but in testing
> > I've found that something really ugly starts showing up when you patch this
> >
> > The symptoms vary, but always are some kind of slab corruption.
> > Here's the last example:
> >
> > =============================================================================
> > BUG kmalloc-256 (Not tainted): Invalid object pointer 0xffff8802407adc60
> > -----------------------------------------------------------------------------
> >
> > Disabling lock debugging due to kernel taint
> > INFO: Slab 0xffffea000901eb00 objects=28 used=22 fp=0xffff8802407ad6d0 flags=0x20000000004081
> > CPU: 1 PID: 1185 Comm: trinity-c1 Tainted: G B 3.15.0-rc1+ #191
> > ffff880243c073c0 00000000f952f249 ffff8800a1a2bc10 ffffffffbd74686d
> > ffffea000901eb00 ffff8800a1a2bce8 ffffffffbd1b0cd4 ffffffff00000020
> > ffff8800a1a2bcf8 ffff8800a1a2bca8 61766e4943c00a18 656a626f2064696c
> > Call Trace:
> > [<ffffffffbd74686d>] dump_stack+0x4e/0x7a
> > [<ffffffffbd1b0cd4>] slab_err+0xb4/0xe0
> > [<ffffffffbd0bf3ae>] ? put_lock_stats.isra.23+0xe/0x30
> > [<ffffffffbd1b0da6>] ? slab_pad_check.part.44+0xa6/0x170
> > [<ffffffffbd744e7f>] free_debug_processing+0x88/0x22a
> > [<ffffffffbd1c7041>] ? compat_do_readv_writev+0xe1/0x250
> > [<ffffffffbd74506d>] __slab_free+0x4c/0x2c3
> > [<ffffffffbd1c6679>] ? do_sync_readv_writev+0x59/0xa0
> > [<ffffffffbd1b2614>] kfree+0x214/0x220
> > [<ffffffffbd1c7041>] ? compat_do_readv_writev+0xe1/0x250
> > [<ffffffffbd1c7041>] compat_do_readv_writev+0xe1/0x250
> > [<ffffffffbd0bf716>] ? lock_release_holdtime.part.24+0xe6/0x160
> > [<ffffffffbd0a3ccd>] ? get_parent_ip+0xd/0x50
> > [<ffffffffbd75642b>] ? preempt_count_sub+0x6b/0xf0
> > [<ffffffffbd751a01>] ? _raw_spin_unlock+0x31/0x50
> > [<ffffffffbd349883>] ? __this_cpu_preempt_check+0x13/0x20
> > [<ffffffffbd1c730a>] compat_writev+0x3a/0x80
> > [<ffffffffbd1c85d8>] compat_SyS_writev+0x58/0xd0
> > [<ffffffffbd75c6a9>] ia32_do_call+0x13/0x13
> > FIX kmalloc-256: Object at 0xffff8802407adc60 not freed
> >
> >
> > I also had an incomplete trace that showed vmsplice causing a bug in mm/slub.c:3396
> > on an earlier run.
> >
> > The crash happens very quickly (within a few seconds of running trinity) for me.
>
> Al, Miklos ?
I probably am blind, but I can't see how that will cause slab corruption.
Have you verified that it's just that single patch and no other change?
Thanks,
Miklos
On Tue, Apr 22, 2014 at 10:42:52AM +0200, Miklos Szeredi wrote:
> > > Your patch looks a lot more complete than the quick hack I did a few
> > > days ago when coverity first started nagging about this, but in testing
> > > I've found that something really ugly starts showing up when you patch this
> > >
> > > The symptoms vary, but always are some kind of slab corruption.
> > > Here's the last example:
> > >
> > > The crash happens very quickly (within a few seconds of running trinity) for me.
> >
> > Al, Miklos ?
>
> I probably am blind, but I can't see how that will cause slab corruption.
>
> Have you verified that it's just that single patch and no other change?
yeah, runs a lot longer (before eventually hitting other issues) without
the patch.
Dave
The proposed patch doesn't work because in compat_rw_copy_check_uvector(), 'iov'
is incremented in the loop before it is freed or returned. This probably should
be changed to indexing with 'seg', like in the non-compat version...
On Wed, Apr 23, 2014 at 12:06:39AM -0500, Eric Biggers wrote:
> The proposed patch doesn't work because in compat_rw_copy_check_uvector(), 'iov'
> is incremented in the loop before it is freed or returned. This probably should
> be changed to indexing with 'seg', like in the non-compat version...
Also, there is still a memory leak in vmsplice() as it does not free the iov
buffer if 0 is returned from rw_copy_check_uvector() (possible if all segments
are of zero length).
On Wed, Apr 23, 2014 at 12:06:39AM -0500, Eric Biggers wrote:
> The proposed patch doesn't work because in compat_rw_copy_check_uvector(),
> 'iov' is incremented in the loop before it is freed or returned. This
> probably should be changed to indexing with 'seg', like in the non-compat
> version...
Do'h, I am indeed blind.
Updated patch below.
Thanks,
Miklos
----
Subject: vfs: rw_copy_check_uvector() - free iov on error
From: Miklos Szeredi <[email protected]>
Some callers (aio_run_iocb, vmsplice_to_user) forget to free the iov on
error. This seems to be a recurring problem, with most callers being buggy
initially.
So instead of fixing the callers, fix the semantics: free the allocated iov
on error, so callers don't have to.
We could return either fast_pointer or NULL for the error case. I've opted
for NULL.
Found by Coverity Scan.
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/compat.c | 24 +++++++++++++++---------
fs/read_write.c | 13 ++++++++++---
2 files changed, 25 insertions(+), 12 deletions(-)
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -689,7 +689,7 @@ ssize_t rw_copy_check_uvector(int type,
}
if (copy_from_user(iov, uvector, nr_segs*sizeof(*uvector))) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
/*
@@ -710,12 +710,12 @@ ssize_t rw_copy_check_uvector(int type,
* it's about to overflow ssize_t */
if (len < 0) {
ret = -EINVAL;
- goto out;
+ goto out_free;
}
if (type >= 0
&& unlikely(!access_ok(vrfy_dir(type), buf, len))) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
if (len > MAX_RW_COUNT - ret) {
len = MAX_RW_COUNT - ret;
@@ -726,6 +726,13 @@ ssize_t rw_copy_check_uvector(int type,
out:
*ret_pointer = iov;
return ret;
+
+out_free:
+ if (iov != fast_pointer) {
+ kfree(iov);
+ iov = NULL;
+ }
+ goto out;
}
static ssize_t do_readv_writev(int type, struct file *file,
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -549,7 +549,7 @@ ssize_t compat_rw_copy_check_uvector(int
struct iovec **ret_pointer)
{
compat_ssize_t tot_len;
- struct iovec *iov = *ret_pointer = fast_pointer;
+ struct iovec *iov = fast_pointer;
ssize_t ret = 0;
int seg;
@@ -570,11 +570,10 @@ ssize_t compat_rw_copy_check_uvector(int
if (iov == NULL)
goto out;
}
- *ret_pointer = iov;
ret = -EFAULT;
if (!access_ok(VERIFY_READ, uvector, nr_segs*sizeof(*uvector)))
- goto out;
+ goto out_free;
/*
* Single unix specification:
@@ -593,27 +592,34 @@ ssize_t compat_rw_copy_check_uvector(int
if (__get_user(len, &uvector->iov_len) ||
__get_user(buf, &uvector->iov_base)) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
if (len < 0) /* size_t not fitting in compat_ssize_t .. */
- goto out;
+ goto out_free;
if (type >= 0 &&
!access_ok(vrfy_dir(type), compat_ptr(buf), len)) {
ret = -EFAULT;
- goto out;
+ goto out_free;
}
if (len > MAX_RW_COUNT - tot_len)
len = MAX_RW_COUNT - tot_len;
tot_len += len;
- iov->iov_base = compat_ptr(buf);
- iov->iov_len = (compat_size_t) len;
+ iov[seg].iov_base = compat_ptr(buf);
+ iov[seg].iov_len = (compat_size_t) len;
uvector++;
- iov++;
}
ret = tot_len;
out:
+ *ret_pointer = iov;
return ret;
+
+out_free:
+ if (iov != fast_pointer) {
+ kfree(iov);
+ iov = NULL;
+ }
+ goto out;
}
static inline long
On Wed, Apr 23, 2014 at 12:25:34AM -0500, Eric Biggers wrote:
> On Wed, Apr 23, 2014 at 12:06:39AM -0500, Eric Biggers wrote:
> > The proposed patch doesn't work because in compat_rw_copy_check_uvector(),
> > 'iov' is incremented in the loop before it is freed or returned. This
> > probably should be changed to indexing with 'seg', like in the non-compat
> > version...
>
> Also, there is still a memory leak in vmsplice() as it does not free the iov
> buffer if 0 is returned from rw_copy_check_uvector() (possible if all segments
> are of zero length).
There are more problems. E.g. count is zero so nothing will be copied. This
function needs some care and attention (and testing).
Thanks,
Miklos