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

From: Darrick J. Wong <[email protected]>

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 <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/xfs_ioctl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_ioctl.c +0 -0

index f193f7b288ca..44e1a290f053 100644

fs/xfs/xfs_ioctl.c +0 -0

 	if (copy_to_user(breq->ubuffer, &ig1, sizeof(struct xfs_inogrp)))

Quoted 18 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

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 <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/xfs_ioctl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_ioctl.c +0 -0

index f193f7b288ca..44e1a290f053 100644

fs/xfs/xfs_ioctl.c xfs_fsbulkstat_one_fmt( · line 724

Quoted 7 lines

fs/xfs/xfs_ioctl.c +1 -1

@@ -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?

Quoted 8 lines

fs/xfs/xfs_ioctl.c +1 -1

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

fs/xfs/xfs_ioctl.c xfs_fsinumbers_fmt( · line 737

Quoted 1 line

fs/xfs/xfs_ioctl.c +0 -0

 	xfs_inumbers_to_inogrp(&ig1, igrp);

... and this function initializes all elements of xfs_inogrp ig1 AFAICT.

Maybe I need more coffee?

From: Darrick J. Wong <[email protected]>

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 <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/xfs_ioctl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_ioctl.c +0 -0

index f193f7b288ca..44e1a290f053 100644

Quoted 12 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

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 <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/xfs_ioctl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/xfs/xfs_ioctl.c xfs_fsbulkstat_one_fmt( · line 722

Quoted 10 lines

fs/xfs/xfs_ioctl.c +1 -1

index f193f7b288ca..44e1a290f053 100644
@@ -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