[PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Dan Rosenberg]

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.  Since all other members are filled in all code
paths and there are no padding bytes in this structure, it's safe to
avoid an expensive memset() in favor of just clearing this one field.

Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
---
 fs/xfs/xfs_fsops.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index cec89dd..17c4785 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -102,6 +102,7 @@ xfs_fs_geometry(
 				mp->m_sb.sb_logsectsize : BBSIZE;
 		geo->rtsectsize = mp->m_sb.sb_blocksize;
 		geo->dirblocksize = mp->m_dirblksize;
+		geo->logsunit = 0;
 	}
 	if (new_version >= 4) {
 		geo->flags |=

[PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Dan Rosenberg]

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.  Since all other members are filled in all code
paths and there are no padding bytes in this structure, it's safe to
avoid an expensive memset() in favor of just clearing this one field.

Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
---
 fs/xfs/xfs_fsops.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index cec89dd..17c4785 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -102,6 +102,7 @@ xfs_fs_geometry(
 				mp->m_sb.sb_logsectsize : BBSIZE;
 		geo->rtsectsize = mp->m_sb.sb_blocksize;
 		geo->dirblocksize = mp->m_dirblksize;
+		geo->logsunit = 0;
 	}
 	if (new_version >= 4) {
 		geo->flags |=


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [Security] [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Eugene Teo]

On Thu, Feb 10, 2011 at 10:25 PM, Dan Rosenberg
<drosenberg@vsecurity.com> wrote:
> 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.  Since all other members are filled in all code
> paths and there are no padding bytes in this structure, it's safe to
> avoid an expensive memset() in favor of just clearing this one field.
>
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>

There are three callers to xfs_fs_geometry() with version number 3 and
4. I don't see any for version number 2, so this looks fine.

Reviewed-by: Eugene Teo <eugeneteo@kernel.org>

Thanks, Eugene

Re: [Security] [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Eugene Teo]

On Thu, Feb 10, 2011 at 10:25 PM, Dan Rosenberg
<drosenberg@vsecurity.com> wrote:
> 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.  Since all other members are filled in all code
> paths and there are no padding bytes in this structure, it's safe to
> avoid an expensive memset() in favor of just clearing this one field.
>
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>

There are three callers to xfs_fs_geometry() with version number 3 and
4. I don't see any for version number 2, so this looks fine.

Reviewed-by: Eugene Teo <eugeneteo@kernel.org>

Thanks, Eugene

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Dave Chinner]

On Thu, Feb 10, 2011 at 09:25:04AM -0500, Dan Rosenberg wrote:
> 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.  Since all other members are filled in all code
> paths and there are no padding bytes in this structure, it's safe to
> avoid an expensive memset() in favor of just clearing this one field.

If this really is a security problem, then it should use a memset.
This is not a performance critical path and there are differences in
the padding of the structure between 32 bit and 64 bit ioctl
variants (it has a compat ioctl handler) and that can only be
correctly handled by memset().

Also, using a memset means we won't have the problem of introducing
new uninitialised fields or padding if we ever rev the structure
again...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Dave Chinner]

On Thu, Feb 10, 2011 at 09:25:04AM -0500, Dan Rosenberg wrote:
> 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.  Since all other members are filled in all code
> paths and there are no padding bytes in this structure, it's safe to
> avoid an expensive memset() in favor of just clearing this one field.

If this really is a security problem, then it should use a memset.
This is not a performance critical path and there are differences in
the padding of the structure between 32 bit and 64 bit ioctl
variants (it has a compat ioctl handler) and that can only be
correctly handled by memset().

Also, using a memset means we won't have the problem of introducing
new uninitialised fields or padding if we ever rev the structure
again...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Dan Rosenberg]

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>
---
 fs/xfs/xfs_fsops.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index cec89dd..85668ef 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/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;

[PATCH v2] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Dan Rosenberg]

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>
---
 fs/xfs/xfs_fsops.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index cec89dd..85668ef 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/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

Re: [PATCH v2] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Eugene Teo]

On Mon, Feb 14, 2011 at 9:45 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> 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>

> ---
>  fs/xfs/xfs_fsops.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index cec89dd..85668ef 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/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;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH v2] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1

[Eugene Teo]

On Mon, Feb 14, 2011 at 9:45 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> 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>

> ---
>  fs/xfs/xfs_fsops.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index cec89dd..85668ef 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/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;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs