xfs
[Top] [All Lists]

Re: kernel panic - stack-protector: kernel stack is corrupted in: f87aca

To: Jeffrey Hundstad <jeffrey.hundstad@xxxxxxxx>
Subject: Re: kernel panic - stack-protector: kernel stack is corrupted in: f87aca93
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 28 Feb 2011 19:32:53 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <4D6C45EE.80203@xxxxxxxx>
References: <4D6C28A5.60905@xxxxxxxx> <4D6C372E.6030105@xxxxxxxxxxx> <4D6C45EE.80203@xxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.13) Gecko/20101207 Thunderbird/3.1.7
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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
>     Reviewed-by: Eugene Teo <eugeneteo@xxxxxxxxxx>
>     Signed-off-by: Alex Elder <aelder@xxxxxxx>
> 
> 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;

<Prev in Thread] Current Thread [Next in Thread>