* 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
* 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
* [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: [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox