* kernel panic - stack-protector: kernel stack is corrupted in: f87aca93
@ 2011-02-28 22:58 Jeffrey Hundstad
2011-03-01 0:00 ` Eric Sandeen
2011-03-01 1:37 ` [PATCH] xfs: zero proper structure size for geometry calls Eric Sandeen
0 siblings, 2 replies; 16+ messages in thread
From: Jeffrey Hundstad @ 2011-02-28 22:58 UTC (permalink / raw)
To: xfs
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.
--
Jeffrey Hundstad
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: kernel panic - stack-protector: kernel stack is corrupted in: f87aca93
2011-02-28 22:58 kernel panic - stack-protector: kernel stack is corrupted in: f87aca93 Jeffrey Hundstad
@ 2011-03-01 0:00 ` Eric Sandeen
2011-03-01 1:03 ` Jeffrey Hundstad
2011-03-01 1:37 ` [PATCH] xfs: zero proper structure size for geometry calls Eric Sandeen
1 sibling, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2011-03-01 0:00 UTC (permalink / raw)
To: Jeffrey Hundstad; +Cc: xfs
On 2/28/11 4:58 PM, Jeffrey Hundstad wrote:
> 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.
>
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?
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: kernel panic - stack-protector: kernel stack is corrupted in: f87aca93
2011-03-01 0:00 ` Eric Sandeen
@ 2011-03-01 1:03 ` Jeffrey Hundstad
2011-03-01 1:32 ` Eric Sandeen
0 siblings, 1 reply; 16+ messages in thread
From: Jeffrey Hundstad @ 2011-03-01 1:03 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On 02/28/2011 06:00 PM, Eric Sandeen wrote:
> On 2/28/11 4:58 PM, Jeffrey Hundstad wrote:
>
>> 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.
>>
>>
> 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 <drosenberg@vsecurity.com>
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 <drosenberg@vsecurity.com>
Reviewed-by: Eugene Teo <eugeneteo@kernel.org>
Signed-off-by: Alex Elder <aelder@sgi.com>
diff --git b/fs/xfs/xfs_fsops.c a/fs/xfs/xfs_fsops.c
index cec89dd..85668ef 100644
--- b/fs/xfs/xfs_fsops.c
+++ a/fs/xfs/xfs_fsops.c
@@ -53,6 +53,9 @@ xfs_fs_geometry(
xfs_fsop_geom_t *geo,
int new_version)
{
+
+ 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;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: kernel panic - stack-protector: kernel stack is corrupted in: f87aca93
2011-03-01 1:03 ` Jeffrey Hundstad
@ 2011-03-01 1:32 ` Eric Sandeen
2011-03-01 2:57 ` Dave Chinner
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2011-03-01 1:32 UTC (permalink / raw)
To: Jeffrey Hundstad; +Cc: xfs
On 2/28/11 7:03 PM, Jeffrey Hundstad wrote:
> 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 :(
-Eric
>
> commit 3a3675b7f23f83ca8c67c9c2b6edf707fd28d1ba
> Author: Dan Rosenberg <drosenberg@vsecurity.com>
> 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 <drosenberg@vsecurity.com>
> Reviewed-by: Eugene Teo <eugeneteo@kernel.org>
> Signed-off-by: Alex Elder <aelder@sgi.com>
>
> diff --git b/fs/xfs/xfs_fsops.c a/fs/xfs/xfs_fsops.c
> index cec89dd..85668ef 100644
> --- b/fs/xfs/xfs_fsops.c
> +++ a/fs/xfs/xfs_fsops.c
> @@ -53,6 +53,9 @@ xfs_fs_geometry(
> xfs_fsop_geom_t *geo,
> int new_version)
> {
> +
> + 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;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] xfs: zero proper structure size for geometry calls
2011-02-28 22:58 kernel panic - stack-protector: kernel stack is corrupted in: f87aca93 Jeffrey Hundstad
2011-03-01 0:00 ` Eric Sandeen
@ 2011-03-01 1:37 ` Eric Sandeen
2011-03-01 2:59 ` Dave Chinner
2011-03-01 6:59 ` [PATCH V2] " Eric Sandeen
1 sibling, 2 replies; 16+ messages in thread
From: Eric Sandeen @ 2011-03-01 1:37 UTC (permalink / raw)
To: Jeffrey Hundstad; +Cc: Dan Rosenberg, Eugene Teo, xfs
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 <jeffrey.hundstad@mnsu.edu>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
index f5e2a19..34e401f 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -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));
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(xfs_fsop_geom_t));
error = xfs_fs_geometry(mp, &fsgeo, 4);
if (error)
return -error;
diff --git a/fs/xfs/linux-2.6/xfs_ioctl32.c b/fs/xfs/linux-2.6/xfs_ioctl32.c
index b3486df..de11e90 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl32.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl32.c
@@ -73,6 +73,7 @@ xfs_compat_ioc_fsgeometry_v1(
xfs_fsop_geom_t fsgeo;
int error;
+ memset(&fsgeo, 0, sizeof(xfs_fsop_geom_t));
error = xfs_fs_geometry(mp, &fsgeo, 3);
if (error)
return -error;
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 85668ef..cec89dd 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -53,9 +53,6 @@ xfs_fs_geometry(
xfs_fsop_geom_t *geo,
int new_version)
{
-
- 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;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: kernel panic - stack-protector: kernel stack is corrupted in: f87aca93
2011-03-01 1:32 ` Eric Sandeen
@ 2011-03-01 2:57 ` Dave Chinner
0 siblings, 0 replies; 16+ messages in thread
From: Dave Chinner @ 2011-03-01 2:57 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Jeffrey Hundstad, xfs
On Mon, Feb 28, 2011 at 07:32:53PM -0600, Eric Sandeen wrote:
> On 2/28/11 7:03 PM, Jeffrey Hundstad wrote:
> > 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 :(
Fmeh - the differences in structure size, alignment and padding on
different platforms was why I suggested that memset() is a
preferable fix to just setting a single field. I didn't look any
further at the patch before it was committed so I didn't catch the
fact that it was busted in the first place...
Just another example of Occam's Eraser(*) in action, I'd say.
Cheers,
Dave
(*) From the Fortune Cookie database:
OCCAM'S ERASER:
The philosophical principle that even the simplest solution is
bound to have something wrong with it.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs: zero proper structure size for geometry calls
2011-03-01 1:37 ` [PATCH] xfs: zero proper structure size for geometry calls Eric Sandeen
@ 2011-03-01 2:59 ` Dave Chinner
2011-03-01 3:01 ` Eric Sandeen
2011-03-01 6:59 ` [PATCH V2] " Eric Sandeen
1 sibling, 1 reply; 16+ messages in thread
From: Dave Chinner @ 2011-03-01 2:59 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Jeffrey Hundstad, Dan Rosenberg, Eugene Teo, xfs
On Mon, Feb 28, 2011 at 07:37:50PM -0600, Eric Sandeen wrote:
> 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 <jeffrey.hundstad@mnsu.edu>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
> index f5e2a19..34e401f 100644
> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
> @@ -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 <dchinner@redhat.com>
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs: zero proper structure size for geometry calls
2011-03-01 2:59 ` Dave Chinner
@ 2011-03-01 3:01 ` Eric Sandeen
0 siblings, 0 replies; 16+ messages in thread
From: Eric Sandeen @ 2011-03-01 3:01 UTC (permalink / raw)
To: Dave Chinner; +Cc: Jeffrey Hundstad, Dan Rosenberg, Eugene Teo, xfs
On 2/28/11 8:59 PM, Dave Chinner wrote:
> On Mon, Feb 28, 2011 at 07:37:50PM -0600, Eric Sandeen wrote:
>> 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 <jeffrey.hundstad@mnsu.edu>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
>> index f5e2a19..34e401f 100644
>> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
>> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
>> @@ -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 <dchinner@redhat.com>
>
> Cheers,
>
> Dave.
Ok, I can resend, I was going to do that but most of xfs uses the other style so wasn't sure.
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2] xfs: zero proper structure size for geometry calls
2011-03-01 1:37 ` [PATCH] xfs: zero proper structure size for geometry calls Eric Sandeen
2011-03-01 2:59 ` Dave Chinner
@ 2011-03-01 6:59 ` Eric Sandeen
2011-03-01 12:55 ` Dan Rosenberg
1 sibling, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2011-03-01 6:59 UTC (permalink / raw)
To: Jeffrey Hundstad; +Cc: Dan Rosenberg, Eugene Teo, xfs
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 <jeffrey.hundstad@mnsu.edu>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: Use sizeof (variable) not sizeof (type)
diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
index f5e2a19..871e5f0 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -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;
diff --git a/fs/xfs/linux-2.6/xfs_ioctl32.c b/fs/xfs/linux-2.6/xfs_ioctl32.c
index b3486df..f25d38e 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl32.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl32.c
@@ -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;
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 85668ef..cec89dd 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -53,9 +53,6 @@ xfs_fs_geometry(
xfs_fsop_geom_t *geo,
int new_version)
{
-
- 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;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V2] xfs: zero proper structure size for geometry calls
2011-03-01 6:59 ` [PATCH V2] " Eric Sandeen
@ 2011-03-01 12:55 ` Dan Rosenberg
2011-03-01 15:36 ` Jeffrey Hundstad
0 siblings, 1 reply; 16+ messages in thread
From: Dan Rosenberg @ 2011-03-01 12:55 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Jeffrey Hundstad, Eugene Teo, xfs
On Tue, 2011-03-01 at 00:59 -0600, Eric Sandeen wrote:
> 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.
Thanks for catching this early, and sorry for the misstep.
Reviewed-by: Dan Rosenberg <drosenberg@vsecurity.com>
>
> Reported-by: Jeffrey Hundstad <jeffrey.hundstad@mnsu.edu>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> V2: Use sizeof (variable) not sizeof (type)
>
> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
> index f5e2a19..871e5f0 100644
> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
> @@ -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;
> diff --git a/fs/xfs/linux-2.6/xfs_ioctl32.c b/fs/xfs/linux-2.6/xfs_ioctl32.c
> index b3486df..f25d38e 100644
> --- a/fs/xfs/linux-2.6/xfs_ioctl32.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl32.c
> @@ -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;
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 85668ef..cec89dd 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -53,9 +53,6 @@ xfs_fs_geometry(
> xfs_fsop_geom_t *geo,
> int new_version)
> {
> -
> - 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;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] xfs: zero proper structure size for geometry calls
2011-03-01 12:55 ` Dan Rosenberg
@ 2011-03-01 15:36 ` Jeffrey Hundstad
2011-03-01 15:49 ` Eric Sandeen
0 siblings, 1 reply; 16+ messages in thread
From: Jeffrey Hundstad @ 2011-03-01 15:36 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: Eric Sandeen, Eugene Teo, xfs
On 03/01/2011 06:55 AM, Dan Rosenberg wrote:
> On Tue, 2011-03-01 at 00:59 -0600, Eric Sandeen wrote:
>
>> 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.
>>
> Thanks for catching this early, and sorry for the misstep.
>
> Reviewed-by: Dan Rosenberg<drosenberg@vsecurity.com>
>
>
>> Reported-by: Jeffrey Hundstad<jeffrey.hundstad@mnsu.edu>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> ---
>>
>> V2: Use sizeof (variable) not sizeof (type)
>>
>> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
>> index f5e2a19..871e5f0 100644
>> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
>> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
>> @@ -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;
>> diff --git a/fs/xfs/linux-2.6/xfs_ioctl32.c b/fs/xfs/linux-2.6/xfs_ioctl32.c
>> index b3486df..f25d38e 100644
>> --- a/fs/xfs/linux-2.6/xfs_ioctl32.c
>> +++ b/fs/xfs/linux-2.6/xfs_ioctl32.c
>> @@ -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;
>> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
>> index 85668ef..cec89dd 100644
>> --- a/fs/xfs/xfs_fsops.c
>> +++ b/fs/xfs/xfs_fsops.c
>> @@ -53,9 +53,6 @@ xfs_fs_geometry(
>> xfs_fsop_geom_t *geo,
>> int new_version)
>> {
>> -
>> - 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 <jeffrey.hundstad@mnsu.edu>
--
Jeffrey Hundstad
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2] xfs: zero proper structure size for geometry calls
2011-03-01 15:36 ` Jeffrey Hundstad
@ 2011-03-01 15:49 ` Eric Sandeen
2011-03-01 17:50 ` [PATCH, V3 (sort of)] " Alex Elder
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2011-03-01 15:49 UTC (permalink / raw)
To: Jeffrey Hundstad; +Cc: Dan Rosenberg, Eugene Teo, xfs
On 3/1/11 9:36 AM, Jeffrey Hundstad wrote:
> 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 <jeffrey.hundstad@mnsu.edu>
>
Thanks for narrowing it down to 1 commit :)
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH, V3 (sort of)] xfs: zero proper structure size for geometry calls
2011-03-01 15:49 ` Eric Sandeen
@ 2011-03-01 17:50 ` Alex Elder
2011-03-01 18:18 ` Eric Sandeen
2011-03-02 0:02 ` Dave Chinner
0 siblings, 2 replies; 16+ messages in thread
From: Alex Elder @ 2011-03-01 17:50 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Jeffrey Hundstad, Dan Rosenberg, Eugene Teo, xfs
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 <jeffrey.hundstad@mnsu.edu>
Signed-off-by: Alex Elder <aelder@sgi.com>
---
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
===================================================================
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -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;
- if (copy_to_user(arg, &fsgeo, sizeof(fsgeo)))
+ /*
+ * Caller should have passed an argument of type
+ * xfs_fsop_geom_v1_t. This is a proper subset of the
+ * xfs_fsop_geom_t that xfs_fs_geometry() fills in.
+ */
+ if (copy_to_user(arg, &fsgeo, sizeof (xfs_fsop_geom_v1_t)))
return -XFS_ERROR(EFAULT);
return 0;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, V3 (sort of)] xfs: zero proper structure size for geometry calls
2011-03-01 17:50 ` [PATCH, V3 (sort of)] " Alex Elder
@ 2011-03-01 18:18 ` Eric Sandeen
2011-03-01 21:40 ` Jeffrey Hundstad
2011-03-02 0:02 ` Dave Chinner
1 sibling, 1 reply; 16+ messages in thread
From: Eric Sandeen @ 2011-03-01 18:18 UTC (permalink / raw)
To: aelder; +Cc: Jeffrey Hundstad, Dan Rosenberg, Eugene Teo, xfs
On 3/1/11 11:50 AM, Alex Elder wrote:
> 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.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
thanks,
-Eric
> -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 <jeffrey.hundstad@mnsu.edu>
> Signed-off-by: Alex Elder <aelder@sgi.com>
>
> ---
> 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
> ===================================================================
> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
> @@ -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;
>
> - if (copy_to_user(arg, &fsgeo, sizeof(fsgeo)))
> + /*
> + * Caller should have passed an argument of type
> + * xfs_fsop_geom_v1_t. This is a proper subset of the
> + * xfs_fsop_geom_t that xfs_fs_geometry() fills in.
> + */
> + if (copy_to_user(arg, &fsgeo, sizeof (xfs_fsop_geom_v1_t)))
> return -XFS_ERROR(EFAULT);
> return 0;
> }
>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, V3 (sort of)] xfs: zero proper structure size for geometry calls
2011-03-01 18:18 ` Eric Sandeen
@ 2011-03-01 21:40 ` Jeffrey Hundstad
0 siblings, 0 replies; 16+ messages in thread
From: Jeffrey Hundstad @ 2011-03-01 21:40 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Dan Rosenberg, xfs, Eugene Teo, aelder
On 03/01/2011 12:18 PM, Eric Sandeen wrote:
> On 3/1/11 11:50 AM, Alex Elder wrote:
>
>> 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.
>>
> Reviewed-by: Eric Sandeen<sandeen@redhat.com>
I can't tell you if the security concerns are met but I can tell you
that xfs_fsr is working as one would expect without a Kernel panic.
Tested-by: Jeffrey Hundstad <jeffrey.hundstad@mnsu.edu>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH, V3 (sort of)] xfs: zero proper structure size for geometry calls
2011-03-01 17:50 ` [PATCH, V3 (sort of)] " Alex Elder
2011-03-01 18:18 ` Eric Sandeen
@ 2011-03-02 0:02 ` Dave Chinner
1 sibling, 0 replies; 16+ messages in thread
From: Dave Chinner @ 2011-03-02 0:02 UTC (permalink / raw)
To: Alex Elder; +Cc: Jeffrey Hundstad, Eric Sandeen, Dan Rosenberg, Eugene Teo, xfs
On Tue, Mar 01, 2011 at 11:50:00AM -0600, Alex Elder wrote:
> 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 <jeffrey.hundstad@mnsu.edu>
> Signed-off-by: Alex Elder <aelder@sgi.com>
>
> ---
> 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
> ===================================================================
> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
> @@ -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;
>
> - if (copy_to_user(arg, &fsgeo, sizeof(fsgeo)))
> + /*
> + * Caller should have passed an argument of type
> + * xfs_fsop_geom_v1_t. This is a proper subset of the
> + * xfs_fsop_geom_t that xfs_fs_geometry() fills in.
> + */
> + if (copy_to_user(arg, &fsgeo, sizeof (xfs_fsop_geom_v1_t)))
> return -XFS_ERROR(EFAULT);
Minor thing: "sizeof(foo)", not "sizeof (foo)"....
Cheers,,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-03-02 0:00 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-28 22:58 kernel panic - stack-protector: kernel stack is corrupted in: f87aca93 Jeffrey Hundstad
2011-03-01 0:00 ` Eric Sandeen
2011-03-01 1:03 ` Jeffrey Hundstad
2011-03-01 1:32 ` Eric Sandeen
2011-03-01 2:57 ` Dave Chinner
2011-03-01 1:37 ` [PATCH] xfs: zero proper structure size for geometry calls Eric Sandeen
2011-03-01 2:59 ` Dave Chinner
2011-03-01 3:01 ` Eric Sandeen
2011-03-01 6:59 ` [PATCH V2] " Eric Sandeen
2011-03-01 12:55 ` Dan Rosenberg
2011-03-01 15:36 ` Jeffrey Hundstad
2011-03-01 15:49 ` Eric Sandeen
2011-03-01 17:50 ` [PATCH, V3 (sort of)] " Alex Elder
2011-03-01 18:18 ` Eric Sandeen
2011-03-01 21:40 ` Jeffrey Hundstad
2011-03-02 0:02 ` Dave Chinner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.