| Message ID | 20190724153545.GC1561054@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: fix stack contents leakage in the v1 bulkstat/inumbers ioctls | expand |
On 7/24/19 10:35 AM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Explicitly initialize the onstack structures to zero so we don't leak > kernel memory into userspace when converting the in-core structure to > the v1 ioctl structure. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_ioctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index f193f7b288ca..44e1a290f053 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -719,7 +719,7 @@ xfs_fsbulkstat_one_fmt( > struct xfs_ibulk *breq, > const struct xfs_bulkstat *bstat) > { > - struct xfs_bstat bs1; > + struct xfs_bstat bs1 = { 0 }; > > xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat); Confused. the first thing xfs_bulkstat_to_bstat does is... void xfs_bulkstat_to_bstat( struct xfs_mount *mp, struct xfs_bstat *bs1, const struct xfs_bulkstat *bstat) { memset(bs1, 0, sizeof(struct xfs_bstat)); so what's leaking here? > if (copy_to_user(breq->ubuffer, &bs1, sizeof(bs1))) > @@ -732,7 +732,7 @@ xfs_fsinumbers_fmt( > struct xfs_ibulk *breq, > const struct xfs_inumbers *igrp) > { > - struct xfs_inogrp ig1; > + struct xfs_inogrp ig1 = { 0 }; > > xfs_inumbers_to_inogrp(&ig1, igrp); ... and this function initializes all elements of xfs_inogrp ig1 AFAICT. Maybe I need more coffee? -Eric > if (copy_to_user(breq->ubuffer, &ig1, sizeof(struct xfs_inogrp))) >
On 7/24/19 3:19 PM, Eric Sandeen wrote: > On 7/24/19 10:35 AM, Darrick J. Wong wrote: >> From: Darrick J. Wong <darrick.wong@oracle.com> >> >> Explicitly initialize the onstack structures to zero so we don't leak >> kernel memory into userspace when converting the in-core structure to >> the v1 ioctl structure. >> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > >> --- >> fs/xfs/xfs_ioctl.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index f193f7b288ca..44e1a290f053 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -719,7 +719,7 @@ xfs_fsbulkstat_one_fmt( >> struct xfs_ibulk *breq, >> const struct xfs_bulkstat *bstat) >> { >> - struct xfs_bstat bs1; >> + struct xfs_bstat bs1 = { 0 }; >> >> xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat); > > Confused. the first thing xfs_bulkstat_to_bstat does is... > > void > xfs_bulkstat_to_bstat( > struct xfs_mount *mp, > struct xfs_bstat *bs1, > const struct xfs_bulkstat *bstat) > { > memset(bs1, 0, sizeof(struct xfs_bstat)); > > so what's leaking here? > >> if (copy_to_user(breq->ubuffer, &bs1, sizeof(bs1))) >> @@ -732,7 +732,7 @@ xfs_fsinumbers_fmt( >> struct xfs_ibulk *breq, >> const struct xfs_inumbers *igrp) >> { >> - struct xfs_inogrp ig1; >> + struct xfs_inogrp ig1 = { 0 }; >> >> xfs_inumbers_to_inogrp(&ig1, igrp); > > ... and this function initializes all elements of xfs_inogrp ig1 AFAICT. > > Maybe I need more coffee? or I need to read mail in order. o_O Ok, struct packing/ hole issues. But let's be consistent, do we zero it out in the conversion function or rely on the caller? (I prefer the former, as is done in xfs_bulkstat_to_bstat, but we surely don't need to do both). Also, Fixes: tag please? -Eric
On Wed, Jul 24, 2019 at 08:35:45AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Explicitly initialize the onstack structures to zero so we don't leak > kernel memory into userspace when converting the in-core structure to > the v1 ioctl structure. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_ioctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index f193f7b288ca..44e1a290f053 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -719,7 +719,7 @@ xfs_fsbulkstat_one_fmt( > struct xfs_ibulk *breq, > const struct xfs_bulkstat *bstat) > { > - struct xfs_bstat bs1; > + struct xfs_bstat bs1 = { 0 }; This sort of initialization is potentially problematic because some versions of GCC will change it as a series of assignments (which doesn't clear the struct hole). It's not clear to me the rules where GCC does this and also I wish there were an option to disable that feature. [ I am still out of office until the end of the month ] regards, dan carpenter
On Thu, Jul 25, 2019 at 09:52:17AM +0300, Dan Carpenter wrote: > On Wed, Jul 24, 2019 at 08:35:45AM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Explicitly initialize the onstack structures to zero so we don't leak > > kernel memory into userspace when converting the in-core structure to > > the v1 ioctl structure. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_ioctl.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > index f193f7b288ca..44e1a290f053 100644 > > --- a/fs/xfs/xfs_ioctl.c > > +++ b/fs/xfs/xfs_ioctl.c > > @@ -719,7 +719,7 @@ xfs_fsbulkstat_one_fmt( > > struct xfs_ibulk *breq, > > const struct xfs_bulkstat *bstat) > > { > > - struct xfs_bstat bs1; > > + struct xfs_bstat bs1 = { 0 }; > > This sort of initialization is potentially problematic because some > versions of GCC will change it as a series of assignments (which doesn't > clear the struct hole). It's not clear to me the rules where GCC does > this and also I wish there were an option to disable that feature. And poor maintainers like me didn't even /know/ that.... ok, I'll go with an explicit memset like Eric suggested in the patch review. --D > [ I am still out of office until the end of the month ] > > regards, > dan carpenter >
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index f193f7b288ca..44e1a290f053 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -719,7 +719,7 @@ xfs_fsbulkstat_one_fmt( struct xfs_ibulk *breq, const struct xfs_bulkstat *bstat) { - struct xfs_bstat bs1; + struct xfs_bstat bs1 = { 0 }; xfs_bulkstat_to_bstat(breq->mp, &bs1, bstat); if (copy_to_user(breq->ubuffer, &bs1, sizeof(bs1))) @@ -732,7 +732,7 @@ xfs_fsinumbers_fmt( struct xfs_ibulk *breq, const struct xfs_inumbers *igrp) { - struct xfs_inogrp ig1; + struct xfs_inogrp ig1 = { 0 }; xfs_inumbers_to_inogrp(&ig1, igrp); if (copy_to_user(breq->ubuffer, &ig1, sizeof(struct xfs_inogrp)))