kernel panic - stack-protector: kernel stack is corrupted in: f87aca93

Hello,

I'm compiling the main Linux branch commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 with no other patches. It boots and seems to operate fine. When I do an xfs_fsr it Kernel panics. I can easily reproduce it

Kernel Panic: (hand copied, photo available)

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs] [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs] [<f87ae062>] ? xfs_file_ioctl+0x33c/0x6fe [xfs] [<c1047f1a>] ? sched_clock_cpu+0x130/0x140 [<c1050f41>] ? trace_hardirqs_off+0xb/0xd [<c1047f57>] ? local_clock+0x2d/0x4e [<c1050f6e>] ? lock_release_holdtime+0x2b/0xcd [<c10a925a>] ? check_valid_pointer+0x1c/0x48 [<c10a99c8>] ? check_object+0x122/0x156 [<f87add26>] ? xfs_file_ioctl+0x0/0x6fe [xfs] [<c10bf5c9>] ? do_vfs_ioctl+0x483/0x4c8 [<c10aab72>] ? kmeme_chache_free+0x8f/0x9b [<c10b41b4>] ? fcheck_files+0xa1/0xd0 [<c10bf64f>] ? sys_ioctl+0x41/0x61 [<c1002893>] ? sysenter_do_call+0x12/0x32

You can find the config, initrd.img, vmlinuz and crash screen at:
http://krypton.mnsu.edu/~j3gum/linux-error-20110228/

- gcc (Debian 4.4.5-13) 4.4.5
- xfs_fsr version 3.1.4

Please let know if you'd like more info.

Quoted 31 lines · Jeffrey Hundstad

Hello,

I'm compiling the main Linux branch commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 with no other patches. It boots and seems to operate fine. When I do an xfs_fsr it Kernel panics. I can easily reproduce it

Kernel Panic: (hand copied, photo available)

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs] [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs] [<f87ae062>] ? xfs_file_ioctl+0x33c/0x6fe [xfs] [<c1047f1a>] ? sched_clock_cpu+0x130/0x140 [<c1050f41>] ? trace_hardirqs_off+0xb/0xd [<c1047f57>] ? local_clock+0x2d/0x4e [<c1050f6e>] ? lock_release_holdtime+0x2b/0xcd [<c10a925a>] ? check_valid_pointer+0x1c/0x48 [<c10a99c8>] ? check_object+0x122/0x156 [<f87add26>] ? xfs_file_ioctl+0x0/0x6fe [xfs] [<c10bf5c9>] ? do_vfs_ioctl+0x483/0x4c8 [<c10aab72>] ? kmeme_chache_free+0x8f/0x9b [<c10b41b4>] ? fcheck_files+0xa1/0xd0 [<c10bf64f>] ? sys_ioctl+0x41/0x61 [<c1002893>] ? sysenter_do_call+0x12/0x32

Quoted 10 lines

You can find the config, initrd.img, vmlinuz and crash screen at:
http://krypton.mnsu.edu/~j3gum/linux-error-20110228/

- gcc (Debian 4.4.5-13) 4.4.5
- xfs_fsr version 3.1.4

Please let know if you'd like more info.

I tried to extract your vmlinuz to a vmlinux to disassemble and look at stack usage of functions, but that was painful and did not seem to work out in the end. :)

Can you do:

objdump -d vmlinux | scripts/checkstack.pl

and similar for xfs if it's a module:

objdump -d xfs.ko | scripts/checkstack.pl

and see how big each of the functions on the above backtrace is, in your kernel?

Sadly, I didn't have the vmlinux file around anymore. I'll be glad to recreate it when I get in tomorrow. However, I have revered commit 3a3675b7f23f83ca8c67c9c2b6edf707fd28d1ba and the problem seems to have vanished. I'm guessing the stack at this point is a little to fragile for a memset. The patch is:

commit 3a3675b7f23f83ca8c67c9c2b6edf707fd28d1ba

Author: Dan Rosenberg <[email protected]>
Date:   Mon Feb 14 13:45:28 2011 +0000

xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

The FSGEOMETRY_V1 ioctl (and its compat equivalent) calls out to xfs_fs_geometry() with a version number of 3. This code path does not fill in the logsunit member of the passed xfs_fsop_geom_t, leading to the leaking of four bytes of uninitialized stack data to potentially unprivileged callers.

v2 switches to memset() to avoid future issues if structure members change, on suggestion of Dave Chinner.

Signed-off-by Dan Rosenberg <[email protected]>

Reviewed-by Eugene Teo <[email protected]>

Signed-off-by Alex Elder <[email protected]>

fs/xfs/xfs_fsops.c +0 -0

index cec89dd..85668ef 100644

Quoted 1 line · Jeffrey Hundstad

Sadly, I didn't have the vmlinux file around anymore. I'll be glad to recreate it when I get in tomorrow. However, I have revered commit 3a3675b7f23f83ca8c67c9c2b6edf707fd28d1ba and the problem seems to have vanished. I'm guessing the stack at this point is a little to fragile for a memset. The patch is:

Ok, no worries, if the below commit is the culprit for sure, that's enough...

So, whoopsies.

STATIC int
xfs_ioc_fsgeometry_v1(
xfs_mount_t *mp,
void __user *arg)

{
        xfs_fsop_geom_v1_t      fsgeo;
        int                     error;

        error = xfs_fs_geometry(mp, (xfs_fsop_geom_t *)&fsgeo, 3);

what we really have is an xfs_fsop_geom_v1_t, but cast to a xfs_fsop_geom_t.

xfs_fs_geometry() zeroes it out to the tune of sizeof (xfs_fsop_geom_t)

the latter is bigger, with the addition of

	__u32           logsunit;

so we overwrite memory that's not ours. :( Seems like we should zero in the callers, when we know how much is really on the stack. I'll follow up with a patch; pity this one was fast-tracked for security, I think :(

commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 added:

+ memset(geo, 0, sizeof(*geo));

but unfortunately we're dealing with a cast pointer here, and the caller may actually have a smaller structure on the stack. Zeroing out more leads to stack corruption traps:

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs]

Fix this by zeroing out the structure in the callers, where we know the actual size.

Reported-by Jeffrey Hundstad <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/linux-2.6/xfs_ioctl.c +0 -0

index f5e2a19..34e401f 100644

Quoted 29 lines · Eric Sandeen

commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 added:

+ memset(geo, 0, sizeof(*geo));

but unfortunately we're dealing with a cast pointer here, and the caller may actually have a smaller structure on the stack. Zeroing out more leads to stack corruption traps:

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs]

Fix this by zeroing out the structure in the callers, where we know the actual size.

Reported-by Jeffrey Hundstad <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/linux-2.6/xfs_ioctl.c +0 -0

index f5e2a19..34e401f 100644

fs/xfs/linux-2.6/xfs_ioctl.c xfs_ioc_fsgeometry_v1( · line 701

Quoted 4 lines

fs/xfs/linux-2.6/xfs_ioctl.c +1 -0

@@ -698,6 +698,7 @@ xfs_ioc_fsgeometry_v1(
 	xfs_fsop_geom_v1_t	fsgeo;
 	int			error;
 
+	memset(&fsgeo, 0, sizeof(xfs_fsop_geom_v1_t));

I'd prefer that sizeof(fsgeo) is used here. That means if the type is changed, then memset doesn't need to be. Same for all the rest of the memset calls.

Otherwise,

Reviewed-by Dave Chinner <[email protected]>

Cheers,

Dave.

commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 added:

+ memset(geo, 0, sizeof(*geo));

but unfortunately we're dealing with a cast pointer here, and the caller may actually have a smaller structure on the stack. Zeroing out more leads to stack corruption traps:

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs]

Fix this by zeroing out the structure in the callers, where we know the actual size.

Reported-by Jeffrey Hundstad <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

V2: Use sizeof (variable) not sizeof (type)

fs/xfs/linux-2.6/xfs_ioctl.c +2 -0

index f5e2a19..871e5f0 100644
@@ -698,6 +698,7 @@ xfs_ioc_fsgeometry_v1(
 	xfs_fsop_geom_v1_t	fsgeo;
 	int			error;
 
+	memset(&fsgeo, 0, sizeof(fsgeo));
 	error = xfs_fs_geometry(mp, (xfs_fsop_geom_t *)&fsgeo, 3);
 	if (error)
 		return -error;

@@ -715,6 +716,7 @@ xfs_ioc_fsgeometry(
 	xfs_fsop_geom_t		fsgeo;
 	int			error;
 
+	memset(&fsgeo, 0, sizeof(fsgeo));
 	error = xfs_fs_geometry(mp, &fsgeo, 4);
 	if (error)
 		return -error;

fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0

index b3486df..f25d38e 100644
@@ -73,6 +73,7 @@ xfs_compat_ioc_fsgeometry_v1(
 	xfs_fsop_geom_t		  fsgeo;
 	int			  error;
 
+	memset(&fsgeo, 0, sizeof(fsgeo));
 	error = xfs_fs_geometry(mp, &fsgeo, 3);
 	if (error)
 		return -error;

fs/xfs/xfs_fsops.c +0 -0

index 85668ef..cec89dd 100644

Quoted 11 lines · Eric Sandeen

commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 added:

+ memset(geo, 0, sizeof(*geo));

but unfortunately we're dealing with a cast pointer here, and the caller may actually have a smaller structure on the stack. Zeroing out more leads to stack corruption traps:

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Quoted 9 lines

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs]

Fix this by zeroing out the structure in the callers, where we know the actual size.

Thanks for catching this early, and sorry for the misstep.

Reviewed-by Dan Rosenberg<[email protected]>

Quoted 45 lines

Reported-by Jeffrey Hundstad<[email protected]>

Signed-off-by Eric Sandeen<[email protected]>

---

V2: Use sizeof (variable) not sizeof (type)

fs/xfs/linux-2.6/xfs_ioctl.c +2 -0

index f5e2a19..871e5f0 100644
@@ -698,6 +698,7 @@ xfs_ioc_fsgeometry_v1(
  	xfs_fsop_geom_v1_t	fsgeo;
  	int			error;
 
+	memset(&fsgeo, 0, sizeof(fsgeo));
  	error = xfs_fs_geometry(mp, (xfs_fsop_geom_t *)&fsgeo, 3);
  	if (error)
  		return -error;

@@ -715,6 +716,7 @@ xfs_ioc_fsgeometry(
  	xfs_fsop_geom_t		fsgeo;
  	int			error;
 
+	memset(&fsgeo, 0, sizeof(fsgeo));
  	error = xfs_fs_geometry(mp,&fsgeo, 4);
  	if (error)
  		return -error;

fs/xfs/linux-2.6/xfs_ioctl32.c +1 -0

index b3486df..f25d38e 100644
@@ -73,6 +73,7 @@ xfs_compat_ioc_fsgeometry_v1(
  	xfs_fsop_geom_t		  fsgeo;
  	int			  error;
 
+	memset(&fsgeo, 0, sizeof(fsgeo));
  	error = xfs_fs_geometry(mp,&fsgeo, 3);
  	if (error)
  		return -error;

fs/xfs/xfs_fsops.c +0 -0

index 85668ef..cec89dd 100644
@@ -53,9 +53,6 @@ xfs_fs_geometry(
  	xfs_fsop_geom_t		*geo,
  	int			new_version)

fs/xfs/xfs_fsops.c xfs_fs_geometry( · line 58

Quoted 8 lines

fs/xfs/xfs_fsops.c +0 -3

  {
-
-	memset(geo, 0, sizeof(*geo));
-
  	geo->blocksize = mp->m_sb.sb_blocksize;
  	geo->rtextsize = mp->m_sb.sb_rextsize;
  	geo->agblocks = mp->m_sb.sb_agblocks;

Hello,

I confirm that this patch DOES FIX the problem I was seeing with xfs_fsr that caused a hit on the stack-protector.

Thanks for your hard work!

Tested-by Jeffrey Hundstad <[email protected]>

I'm sorry to muddy the waters with this. But I think the proposed patch fixes the wrong problem. Having xfs_fs_geometry() zero its argument is fine--it defines an interface and honors it. The real problem lies in xfs_ioc_fsgeometry_v1(), which violates that interface by passing the address of an object that's not the right size. So below is an alternative to Eric's solution which just fixes this one caller instead.

Eric has already told me this makes more sense. It would be nice if Jeffrey would re-test this fix, and Dan would sign off on it as well.

-Alex

Commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 added this call to xfs_fs_geometry() in order to avoid passing kernel stack data back to user space:

+ memset(geo, 0, sizeof(*geo));

Unfortunately, one of the callers of that function passes the address of a smaller data type, cast to fit the type that xfs_fs_geometry() requires. As a result, this can happen:

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted

in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs]

Fix this by fixing that one caller to pass the right type and then copy out the subset it is interested in.

Note: This patch is an alternative to one originally proposed by Eric Sandeen.

Reported-by Jeffrey Hundstad <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
 fs/xfs/linux-2.6/xfs_ioctl.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: b/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================

fs/xfs/linux-2.6/xfs_ioctl.c +2 -2

@@ -695,14 +695,19 @@ xfs_ioc_fsgeometry_v1(
        xfs_mount_t             *mp,
        void                    __user *arg)
 {
-       xfs_fsop_geom_v1_t      fsgeo;
+       xfs_fsop_geom_t         fsgeo;
        int                     error;
 
-       error = xfs_fs_geometry(mp, (xfs_fsop_geom_t *)&fsgeo, 3);
+       error = xfs_fs_geometry(mp, &fsgeo, 3);
        if (error)
                return -error;
 

Quoted 8 lines · Alex Elder

I'm sorry to muddy the waters with this. But I think the proposed patch fixes the wrong problem. Having xfs_fs_geometry() zero its argument is fine--it defines an interface and honors it. The real problem lies in xfs_ioc_fsgeometry_v1(), which violates that interface by passing the address of an object that's not the right size. So below is an alternative to Eric's solution which just fixes this one caller instead.

Quoted 4 lines

Eric has already told me this makes more sense. It would be nice if Jeffrey would re-test this fix, and Dan would sign off on it as well.

Reviewed-by Eric Sandeen<[email protected]>

I'm sorry to muddy the waters with this. But I think the proposed patch fixes the wrong problem. Having xfs_fs_geometry() zero its argument is fine--it defines an interface and honors it. The real problem lies in xfs_ioc_fsgeometry_v1(), which violates that interface by passing the address of an object that's not the right size. So below is an alternative to Eric's solution which just fixes this one caller instead.

Eric has already told me this makes more sense. It would be nice if Jeffrey would re-test this fix, and Dan would sign off on it as well.

-Alex

Commit 493f3358cb289ccf716c5a14fa5bb52ab75943e5 added this call to xfs_fs_geometry() in order to avoid passing kernel stack data back to user space:

+ memset(geo, 0, sizeof(*geo));

Unfortunately, one of the callers of that function passes the address of a smaller data type, cast to fit the type that xfs_fs_geometry() requires. As a result, this can happen:

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted

in: f87aca93

Pid: 262, comm: xfs_fsr Not tainted 2.6.38-rc6-493f3358cb2+ #1

Call Trace:

[<c12991ac>] ? panic+0x50/0x150
[<c102ed71>] ? __stack_chk_fail+0x10/0x18 [<f87aca93>] ? xfs_ioc_fsgeometry_v1+0x56/0x5d [xfs]

Fix this by fixing that one caller to pass the right type and then copy out the subset it is interested in.

Note: This patch is an alternative to one originally proposed by Eric Sandeen.

Reported-by Jeffrey Hundstad <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
 fs/xfs/linux-2.6/xfs_ioctl.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: b/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================

fs/xfs/linux-2.6/xfs_ioctl.c +2 -2

@@ -695,14 +695,19 @@ xfs_ioc_fsgeometry_v1(
        xfs_mount_t             *mp,
        void                    __user *arg)
 {
-       xfs_fsop_geom_v1_t      fsgeo;
+       xfs_fsop_geom_t         fsgeo;
        int                     error;
 
-       error = xfs_fs_geometry(mp, (xfs_fsop_geom_t *)&fsgeo, 3);
+       error = xfs_fs_geometry(mp, &fsgeo, 3);
        if (error)