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 <[email protected]>
---
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 |=
On Thu, Feb 10, 2011 at 10:25 PM, Dan Rosenberg
<[email protected]> 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 <[email protected]>
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 <[email protected]>
Thanks, Eugene
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
[email protected]
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 <[email protected]>
---
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;
On Mon, Feb 14, 2011 at 9:45 PM, Dan Rosenberg <[email protected]> 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 <[email protected]>
Reviewed-by: Eugene Teo <[email protected]>
> ---
> ?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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>