LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1
@ 2011-02-10 14:25 Dan Rosenberg
  2011-02-14  8:41 ` [Security] " Eugene Teo
  2011-02-14 11:46 ` Dave Chinner
  0 siblings, 2 replies; 5+ messages in thread
From: Dan Rosenberg @ 2011-02-10 14:25 UTC (permalink / raw)
  To: aelder, xfs-masters, xfs; +Cc: linux-kernel, stable, security

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 |=



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Security] [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1
  2011-02-10 14:25 [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1 Dan Rosenberg
@ 2011-02-14  8:41 ` Eugene Teo
  2011-02-14 11:46 ` Dave Chinner
  1 sibling, 0 replies; 5+ messages in thread
From: Eugene Teo @ 2011-02-14  8:41 UTC (permalink / raw)
  To: Dan Rosenberg
  Cc: aelder, xfs-masters, xfs, security, linux-kernel, stable, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1
  2011-02-10 14:25 [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1 Dan Rosenberg
  2011-02-14  8:41 ` [Security] " Eugene Teo
@ 2011-02-14 11:46 ` Dave Chinner
  2011-02-14 13:45   ` [PATCH v2] " Dan Rosenberg
  1 sibling, 1 reply; 5+ messages in thread
From: Dave Chinner @ 2011-02-14 11:46 UTC (permalink / raw)
  To: Dan Rosenberg; +Cc: aelder, xfs-masters, xfs, linux-kernel, stable, security

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1
  2011-02-14 11:46 ` Dave Chinner
@ 2011-02-14 13:45   ` Dan Rosenberg
  2011-02-14 23:39     ` Eugene Teo
  0 siblings, 1 reply; 5+ messages in thread
From: Dan Rosenberg @ 2011-02-14 13:45 UTC (permalink / raw)
  To: Dave Chinner, aelder, xfs-masters, xfs; +Cc: linux-kernel, stable, security

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;



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1
  2011-02-14 13:45   ` [PATCH v2] " Dan Rosenberg
@ 2011-02-14 23:39     ` Eugene Teo
  0 siblings, 0 replies; 5+ messages in thread
From: Eugene Teo @ 2011-02-14 23:39 UTC (permalink / raw)
  To: Dan Rosenberg
  Cc: Dave Chinner, aelder, xfs-masters, xfs, linux-kernel, stable, security

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/
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-02-14 23:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-10 14:25 [PATCH] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1 Dan Rosenberg
2011-02-14  8:41 ` [Security] " Eugene Teo
2011-02-14 11:46 ` Dave Chinner
2011-02-14 13:45   ` [PATCH v2] " Dan Rosenberg
2011-02-14 23:39     ` Eugene Teo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).