[PATCH] xfs: fix stack contents leakage in the v1 bulkstat/inumbers ioctls

[PATCH] xfs: fix stack contents leakage in the v1 bulkstat/inumbers ioctls

[Darrick J. Wong]

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);
 	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)))

Re: [PATCH] xfs: fix stack contents leakage in the v1 bulkstat/inumbers ioctls

[Eric Sandeen]

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



> 

Re: [PATCH] xfs: fix stack contents leakage in the v1 bulkstat/inumbers ioctls

[Eric Sandeen]

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

Re: [PATCH] xfs: fix stack contents leakage in the v1 bulkstat/inumbers ioctls

[Dan Carpenter]

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

Re: [PATCH] xfs: fix stack contents leakage in the v1 bulkstat/inumbers ioctls

[Darrick J. Wong]

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
>