From: Phillip Lougher <plougher@redhat.com>
Date: Thu, 2 Jun 2011 20:55:37 -0400
Subject: [fs] xfs: prevent leaking uninit stack memory in FSGEOMETRY_V1 p2
Message-id: <20110602205537.C737444B5D@plougher.csb>
Patchwork-id: 4585
O-Subject: [kernel team] [RHEL5.7 PATCH 2/2] xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1
Bugzilla: 677266
CVE: CVE-2011-0711
BZ #677266
Upsteam status:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=af24ee9ea8d532e16883251a6684dfa1be8eec29
This is the second of two upstream patches which fix CVE-2011-0711
xfs: prevent leaking uninitialized stack memory in FSGEOMETRY_V1
The xfs FSGEOMETRY_v1 ioctl calls xfs_fs_geometry() with an
xfs_fsop_geom_v1_t structure allocated on the stack. On x86_64 this
structure is padded by 4 bytes (108 bytes rounded to 112 bytes), and these bytes
are not initialized by xfs_fs_geometry() and are thus returned uninitialized to
user-space.
This fix is split across two patches upstream due to mistakes in the first
patch, which described the problem as:
"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."
This is confused...
The existing FSGEOMETRY_V1 ioctl function (xfs_ioc_fsgeometry_v1) calls
xfs_fs_geometry() with an xfs_fsop_geom_v1_t structure on the stack cast to a
xfs_fsop_geom_t structure, i.e.
xfs_fsop_geom_v1_t fsgeo;
error = xfs_fs_geometry(mp, (xfs_fsop_geom_t *)&fsgeo, 3);
An xfs_fsop_geom_v1_t structure is the same as the xfs_fsop_geom structure
except it lacks the logsunit field at the end (i.e. the underlying size of the
xfs_fsop_geom_v1 structure is 108 bytes, the xfs_fsop_geom structure is 112
bytes, and logsunit is at the byte offset of 108). So the fact the version
number 3 code path doesn't fill in logsunit is correct (as it will over-write
the stack). On 32 bit machines this is entirely correct, there is no leak.
However, on x86_64 the xfs_fsop_geom_v1_t structure is padded to 112 bytes (4
bytes of padding), and this padding is returned to user-space unintialized.
The first patch is thus broken in that it over-writes the stack on
32-bit machines.
This second patch does the right thing, by converting all callers to pass a
xfs_fsop_geom_t structure, and then memsets the entire structure in
xfs_fs_geometry, avoiding padding issues on 64 bit machines, and stack
over-runs on 32 bit machines.
The compat ioctl (xfs_compat_ioc_fsgeometry_v1) allocates a xfs_fsop_geom on
the stack and passes this to xfs_fs_geometry, but when it is returned to the
user it is truncated to the size of the compat_xfs_fsop_geom_v1 structure.
This structure is _packed_ and it is 108 bytes in size. Therefore the
uninitialised 4 bytes of padding is not returned to user-space.
Testing: existing FSGEOMETRY_V1 ioctl leak confirmed with test program, no
leaks found on patched kernel.
Original commit info
From 0eba576d17f04fd5fb77226f4a03a85eec1a2dea Mon Sep 17 00:00:00 2001
From: Alex Elder <aelder@sgi.com>
Date: Tue, 1 Mar 2011 17:50:00 +0000
Subject: [PATCH 2/2] xfs: zero proper structure size for geometry calls
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>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Tested-by: Jeffrey Hundstad <jeffrey.hundstad@mnsu.edu>
Signed-off-by: Phillip Lougher <plougher@redhat.com>
diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
index ff0cb3d..517b8db 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -762,14 +762,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;
}
