Mimi,
Although this occurs in the xen tree head, it's more likely related to
ima_inode_post_setattr().
tree: git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git devel/for-linus-3.8
head: d01b193de1a961e028ee6df5ad0b34c3e56a995f
commit: d01b193de1a961e028ee6df5ad0b34c3e56a995f [0/0] xen/acpi: Support ACPI hotplug of CPUs.
[ 115.499469] Write protecting the kernel read-only data: 2752k
[ 134.707390] ------------[ cut here ]------------
[ 134.707390] kernel BUG at /c/kernel-tests/src/linux/mm/slub.c:3479!
[ 134.707390] invalid opcode: 0000 [#1]
[ 134.707390] Pid: 1, comm: init Not tainted 3.6.0-00451-gd01b193 #2 Bochs Bochs
[ 134.707390] EIP: 0060:[<c1235dbe>] EFLAGS: 00000246 CPU: 0
[ 134.707390] EIP is at kfree+0x2be/0x490
[ 134.707390] EAX: 00000001 EBX: 00000001 ECX: 00000000 EDX: 00000000
[ 134.707390] ESI: cdeef760 EDI: cd83bdac EBP: cd83bd70 ESP: cd83bd50
[ 134.707390] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 134.707390] CR0: 80050033 CR2: b7782000 CR3: 0f9a8000 CR4: 00000690
[ 134.707390] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 134.707390] DR6: 00000000 DR7: 00000000
[ 134.707390] Process init (pid: 1, ti=cd83a000 task=cd838000 task.ti=cd83a000)
[ 134.707390] Stack:
[ 134.707390] 00000000 cd83bd5c c1085b74 cd83bd88 c1298a5e cd83a000 ffffffc3 cd83bd90
[ 134.707390] cd83bd88 c1298a5e cd83bd90 c1ffdf63 00000001 cd832914 cd83bd98 c1298d4a
[ 134.707390] 00000000 00000002 cd83bdac c11e0400 c995c980 cd832914 00000000 cd83bdc4
[ 134.707390] Call Trace:
[ 134.707390] [<c1085b74>] ? irq_exit+0x74/0x180
[ 134.707390] [<c1298a5e>] ? __simple_xattr_set+0x28e/0x360
[ 134.707390] [<c1298a5e>] __simple_xattr_set+0x28e/0x360
[ 134.707390] [<c1298d4a>] simple_xattr_remove+0x2a/0x40
[ 134.707390] [<c11e0400>] shmem_removexattr+0xc0/0x120
[ 134.707390] [<c14f11ed>] ima_inode_post_setattr+0x21d/0x250
[ 134.707390] [<c1284316>] notify_change+0xb66/0xc40
[ 134.707390] [<c1239c58>] do_truncate+0xf8/0x160
[ 134.707390] [<c1261fcd>] do_last.isra.38+0xb3d/0x1a80
[ 134.707390] [<c12632e1>] path_openat+0x3d1/0xd10
[ 134.707390] [<c11476cb>] ? note_interrupt+0x4db/0x7f0
[ 134.707390] [<c1263c57>] do_filp_open+0x37/0x140
[ 134.707390] [<c123ce71>] do_sys_open+0x471/0x630
[ 134.707390] [<c10881b6>] ? __find_resource+0x166/0x470
[ 134.707390] [<c123d06b>] sys_open+0x3b/0x70
[ 134.707390] [<c1de6ce1>] sysenter_do_call+0x12/0x2c
[ 134.707390] Code: 89 d0 f3 ff 8b 04 9d 8c a0 2c c2 83 c0 01 85 db 89 04 9d 8c a0 2c c2 0f 84 6a 01 00 00 83 05 a8 4b 61 c2 01 83 15 ac 4b 61 c2 00 <0f> 0b 83 05 b8 4b 61 c2 01 83 15 bc 4b 61 c2 00 83 05 30 4c 61
[ 134.707390] EIP: [<c1235dbe>] kfree+0x2be/0x490 SS:ESP 0068:cd83bd50
Thanks,
Fengguang
On Thu, 18 Oct 2012, Fengguang Wu wrote:
> Mimi,
>
> Although this occurs in the xen tree head, it's more likely related to
> ima_inode_post_setattr().
>
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git devel/for-linus-3.8
> head: d01b193de1a961e028ee6df5ad0b34c3e56a995f
> commit: d01b193de1a961e028ee6df5ad0b34c3e56a995f [0/0] xen/acpi: Support ACPI hotplug of CPUs.
>
> [ 115.499469] Write protecting the kernel read-only data: 2752k
> [ 134.707390] ------------[ cut here ]------------
> [ 134.707390] kernel BUG at /c/kernel-tests/src/linux/mm/slub.c:3479!
> [ 134.707390] invalid opcode: 0000 [#1]
> [ 134.707390] Pid: 1, comm: init Not tainted 3.6.0-00451-gd01b193 #2 Bochs Bochs
> [ 134.707390] EIP: 0060:[<c1235dbe>] EFLAGS: 00000246 CPU: 0
> [ 134.707390] EIP is at kfree+0x2be/0x490
> [ 134.707390] EAX: 00000001 EBX: 00000001 ECX: 00000000 EDX: 00000000
> [ 134.707390] ESI: cdeef760 EDI: cd83bdac EBP: cd83bd70 ESP: cd83bd50
> [ 134.707390] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
> [ 134.707390] CR0: 80050033 CR2: b7782000 CR3: 0f9a8000 CR4: 00000690
> [ 134.707390] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 134.707390] DR6: 00000000 DR7: 00000000
> [ 134.707390] Process init (pid: 1, ti=cd83a000 task=cd838000 task.ti=cd83a000)
> [ 134.707390] Stack:
> [ 134.707390] 00000000 cd83bd5c c1085b74 cd83bd88 c1298a5e cd83a000 ffffffc3 cd83bd90
> [ 134.707390] cd83bd88 c1298a5e cd83bd90 c1ffdf63 00000001 cd832914 cd83bd98 c1298d4a
> [ 134.707390] 00000000 00000002 cd83bdac c11e0400 c995c980 cd832914 00000000 cd83bdc4
> [ 134.707390] Call Trace:
> [ 134.707390] [<c1085b74>] ? irq_exit+0x74/0x180
> [ 134.707390] [<c1298a5e>] ? __simple_xattr_set+0x28e/0x360
> [ 134.707390] [<c1298a5e>] __simple_xattr_set+0x28e/0x360
> [ 134.707390] [<c1298d4a>] simple_xattr_remove+0x2a/0x40
> [ 134.707390] [<c11e0400>] shmem_removexattr+0xc0/0x120
> [ 134.707390] [<c14f11ed>] ima_inode_post_setattr+0x21d/0x250
This is a scary one because it involves kfree() of memory that is not
allocated through the slab allocator, and not bypassed because its too
large or otherwise we would have PageCompound() set.
Examining __simple_xattr_set(), though, we know flags has XATTR_REPLACE
set and value == NULL since it's being called from simple_xattr_remove().
That means the local new_xattr variable is initialized to itself, which is
just going to be anything that happens to be on the stack. If the name
passed to simple_xattr_remove() isn't found in the list of xattrs, then
this will call kfree(xattr) which was never set to anything else.
This was changed in commit 38f38657444d ("xattr: extract simple_xattr code
from tmpfs") and the old code actually properly initialized new_xattr to
NULL in which case this would simply fail with -ENODATA instead of causing
the BUG().
So while it's probably not advised to call simple_xattr_remove() for a
name that does not exist in the xattr list, this seems to be as a result
of that commit rather than anything from ima_inode_post_setattr().
Let's add Aristeu Rozanski, Li, Hugh, and Tejun to the cc list.
On Thu, 2012-10-18 at 10:43 +0800, Fengguang Wu wrote:
> Mimi,
>
> Although this occurs in the xen tree head, it's more likely related to
> ima_inode_post_setattr().
Under certain circumstances ima_inode_post_setattr() removes
'security.ima' without checking that it exists. shmem doesn't seem to
like it.
Mimi
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git devel/for-linus-3.8
> head: d01b193de1a961e028ee6df5ad0b34c3e56a995f
> commit: d01b193de1a961e028ee6df5ad0b34c3e56a995f [0/0] xen/acpi: Support ACPI hotplug of CPUs.
>
> [ 115.499469] Write protecting the kernel read-only data: 2752k
> [ 134.707390] ------------[ cut here ]------------
> [ 134.707390] kernel BUG at /c/kernel-tests/src/linux/mm/slub.c:3479!
> [ 134.707390] invalid opcode: 0000 [#1]
> [ 134.707390] Pid: 1, comm: init Not tainted 3.6.0-00451-gd01b193 #2 Bochs Bochs
> [ 134.707390] EIP: 0060:[<c1235dbe>] EFLAGS: 00000246 CPU: 0
> [ 134.707390] EIP is at kfree+0x2be/0x490
> [ 134.707390] EAX: 00000001 EBX: 00000001 ECX: 00000000 EDX: 00000000
> [ 134.707390] ESI: cdeef760 EDI: cd83bdac EBP: cd83bd70 ESP: cd83bd50
> [ 134.707390] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
> [ 134.707390] CR0: 80050033 CR2: b7782000 CR3: 0f9a8000 CR4: 00000690
> [ 134.707390] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 134.707390] DR6: 00000000 DR7: 00000000
> [ 134.707390] Process init (pid: 1, ti=cd83a000 task=cd838000 task.ti=cd83a000)
> [ 134.707390] Stack:
> [ 134.707390] 00000000 cd83bd5c c1085b74 cd83bd88 c1298a5e cd83a000 ffffffc3 cd83bd90
> [ 134.707390] cd83bd88 c1298a5e cd83bd90 c1ffdf63 00000001 cd832914 cd83bd98 c1298d4a
> [ 134.707390] 00000000 00000002 cd83bdac c11e0400 c995c980 cd832914 00000000 cd83bdc4
> [ 134.707390] Call Trace:
> [ 134.707390] [<c1085b74>] ? irq_exit+0x74/0x180
> [ 134.707390] [<c1298a5e>] ? __simple_xattr_set+0x28e/0x360
> [ 134.707390] [<c1298a5e>] __simple_xattr_set+0x28e/0x360
> [ 134.707390] [<c1298d4a>] simple_xattr_remove+0x2a/0x40
> [ 134.707390] [<c11e0400>] shmem_removexattr+0xc0/0x120
> [ 134.707390] [<c14f11ed>] ima_inode_post_setattr+0x21d/0x250
> [ 134.707390] [<c1284316>] notify_change+0xb66/0xc40
> [ 134.707390] [<c1239c58>] do_truncate+0xf8/0x160
> [ 134.707390] [<c1261fcd>] do_last.isra.38+0xb3d/0x1a80
> [ 134.707390] [<c12632e1>] path_openat+0x3d1/0xd10
> [ 134.707390] [<c11476cb>] ? note_interrupt+0x4db/0x7f0
> [ 134.707390] [<c1263c57>] do_filp_open+0x37/0x140
> [ 134.707390] [<c123ce71>] do_sys_open+0x471/0x630
> [ 134.707390] [<c10881b6>] ? __find_resource+0x166/0x470
> [ 134.707390] [<c123d06b>] sys_open+0x3b/0x70
> [ 134.707390] [<c1de6ce1>] sysenter_do_call+0x12/0x2c
> [ 134.707390] Code: 89 d0 f3 ff 8b 04 9d 8c a0 2c c2 83 c0 01 85 db 89 04 9d 8c a0 2c c2 0f 84 6a 01 00 00 83 05 a8 4b 61 c2 01 83 15 ac 4b 61 c2 00 <0f> 0b 83 05 b8 4b 61 c2 01 83 15 bc 4b 61 c2 00 83 05 30 4c 61
> [ 134.707390] EIP: [<c1235dbe>] kfree+0x2be/0x490 SS:ESP 0068:cd83bd50
>
> Thanks,
> Fengguang
Commit 38f38657444d ("xattr: extract simple_xattr code from tmpfs") moved
some code from tmpfs but introduced a subtle bug along the way.
If the name passed to simple_xattr_remove() does not exist in the list of
xattrs, then it is possible to call kfree(new_xattr) when new_xattr is
actually initialized to itself on the stack via uninitialized_var().
This causes a BUG() since the memory was not allocated via the slab
allocator and was not bypassed through to the page allocator because it
was too large.
Initialize the local variable to NULL so the kfree() never takes place.
Reported-by: Fengguang Wu <[email protected]>
Signed-off-by: David Rientjes <[email protected]>
---
fs/xattr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xattr.c b/fs/xattr.c
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -846,7 +846,7 @@ static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
const void *value, size_t size, int flags)
{
struct simple_xattr *xattr;
- struct simple_xattr *uninitialized_var(new_xattr);
+ struct simple_xattr *new_xattr = NULL;
int err = 0;
/* value == NULL means remove */
On Wed, 17 Oct 2012, David Rientjes wrote:
> Commit 38f38657444d ("xattr: extract simple_xattr code from tmpfs") moved
> some code from tmpfs but introduced a subtle bug along the way.
>
> If the name passed to simple_xattr_remove() does not exist in the list of
> xattrs, then it is possible to call kfree(new_xattr) when new_xattr is
> actually initialized to itself on the stack via uninitialized_var().
>
> This causes a BUG() since the memory was not allocated via the slab
> allocator and was not bypassed through to the page allocator because it
> was too large.
>
> Initialize the local variable to NULL so the kfree() never takes place.
>
> Reported-by: Fengguang Wu <[email protected]>
> Signed-off-by: David Rientjes <[email protected]>
Acked-by: Hugh Dickins <[email protected]>
Thank you both: a fine example of the danger of uninitialized_var()!
> ---
> fs/xattr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -846,7 +846,7 @@ static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> const void *value, size_t size, int flags)
> {
> struct simple_xattr *xattr;
> - struct simple_xattr *uninitialized_var(new_xattr);
> + struct simple_xattr *new_xattr = NULL;
> int err = 0;
>
> /* value == NULL means remove */
>
On Wed, Oct 17, 2012 at 08:55:49PM -0700, Hugh Dickins wrote:
> On Wed, 17 Oct 2012, David Rientjes wrote:
>
> > Commit 38f38657444d ("xattr: extract simple_xattr code from tmpfs") moved
> > some code from tmpfs but introduced a subtle bug along the way.
> >
> > If the name passed to simple_xattr_remove() does not exist in the list of
> > xattrs, then it is possible to call kfree(new_xattr) when new_xattr is
> > actually initialized to itself on the stack via uninitialized_var().
> >
> > This causes a BUG() since the memory was not allocated via the slab
> > allocator and was not bypassed through to the page allocator because it
> > was too large.
> >
> > Initialize the local variable to NULL so the kfree() never takes place.
> >
> > Reported-by: Fengguang Wu <[email protected]>
> > Signed-off-by: David Rientjes <[email protected]>
>
> Acked-by: Hugh Dickins <[email protected]>
>
> Thank you both: a fine example of the danger of uninitialized_var()!
Applied.
Hi David,
On Thu, Oct 18, 2012 at 11:41 AM, David Rientjes <[email protected]> wrote:
> Commit 38f38657444d ("xattr: extract simple_xattr code from tmpfs") moved
> some code from tmpfs but introduced a subtle bug along the way.
>
That commit is fine but you did revert commit b9d6cfdeaf67(xattr: mark
variable as uninitialized to make both gcc and smatch happy), too late
to spin?
Hillf
> If the name passed to simple_xattr_remove() does not exist in the list of
> xattrs, then it is possible to call kfree(new_xattr) when new_xattr is
> actually initialized to itself on the stack via uninitialized_var().
>
> This causes a BUG() since the memory was not allocated via the slab
> allocator and was not bypassed through to the page allocator because it
> was too large.
>
> Initialize the local variable to NULL so the kfree() never takes place.
>
> Reported-by: Fengguang Wu <[email protected]>
> Signed-off-by: David Rientjes <[email protected]>
> ---
> fs/xattr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -846,7 +846,7 @@ static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> const void *value, size_t size, int flags)
> {
> struct simple_xattr *xattr;
> - struct simple_xattr *uninitialized_var(new_xattr);
> + struct simple_xattr *new_xattr = NULL;
> int err = 0;
>
> /* value == NULL means remove */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
On Wed, Oct 17, 2012 at 08:41:15PM -0700, David Rientjes wrote:
> Commit 38f38657444d ("xattr: extract simple_xattr code from tmpfs") moved
> some code from tmpfs but introduced a subtle bug along the way.
>
> If the name passed to simple_xattr_remove() does not exist in the list of
> xattrs, then it is possible to call kfree(new_xattr) when new_xattr is
> actually initialized to itself on the stack via uninitialized_var().
>
> This causes a BUG() since the memory was not allocated via the slab
> allocator and was not bypassed through to the page allocator because it
> was too large.
>
> Initialize the local variable to NULL so the kfree() never takes place.
>
> Reported-by: Fengguang Wu <[email protected]>
> Signed-off-by: David Rientjes <[email protected]>
> ---
> fs/xattr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -846,7 +846,7 @@ static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> const void *value, size_t size, int flags)
> {
> struct simple_xattr *xattr;
> - struct simple_xattr *uninitialized_var(new_xattr);
> + struct simple_xattr *new_xattr = NULL;
> int err = 0;
>
> /* value == NULL means remove */
I think Sasha Levin was working on that (Cc'd) along with the smatch fix.
Anyway, __simple_xattr_set is too confusing and rewriting it is in my TODO list
which will shut up smatch too.
Acked-by: Aristeu Rozanski <[email protected]>
--
Aristeu