[PATCH 0/2] statfs: Enforce statfs[64] structure intialization

[PATCH 0/2] statfs: Enforce statfs[64] structure intialization

[Ilya Leoshkevich]

This series fixes copying of uninitialized memory to userspace by
do_statfs_native() and do_statfs64() on s390.

Patch 1 fixes the problem by making the code similar to
put_compat_statfs() and put_compat_statfs64().

Patch 2 gets rid of the padding which caused the issue; even though it
may be considered redundant, it documents that s390 de-facto has an
extra f_spare array element.

Ilya Leoshkevich (2):
  statfs: Enforce statfs[64] structure intialization
  s390/uapi: Cover statfs padding by growing f_spare

 arch/s390/include/asm/compat.h      | 2 +-
 arch/s390/include/uapi/asm/statfs.h | 4 ++--
 fs/statfs.c                         | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.40.1

[PATCH 1/2] statfs: Enforce statfs[64] structure intialization

[Ilya Leoshkevich]

s390's struct statfs and struct statfs64 contain padding, which
field-by-field copying does not set. Initialize the respective structs
with zeros before filling them and copying them to userspace, like it's
already done for the compat versions of these structs.

Found by KMSAN.

Acked-by: Heiko Carstens <hca@linux.ibm.com>
Cc: stable@vger.kernel.org # v4.14+
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 fs/statfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/statfs.c b/fs/statfs.c
index 0ba34c135593..96d1c3edf289 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -130,6 +130,7 @@ static int do_statfs_native(struct kstatfs *st, struct statfs __user *p)
 	if (sizeof(buf) == sizeof(*st))
 		memcpy(&buf, st, sizeof(*st));
 	else {
+		memset(&buf, 0, sizeof(buf));
 		if (sizeof buf.f_blocks == 4) {
 			if ((st->f_blocks | st->f_bfree | st->f_bavail |
 			     st->f_bsize | st->f_frsize) &
@@ -158,7 +159,6 @@ static int do_statfs_native(struct kstatfs *st, struct statfs __user *p)
 		buf.f_namelen = st->f_namelen;
 		buf.f_frsize = st->f_frsize;
 		buf.f_flags = st->f_flags;
-		memset(buf.f_spare, 0, sizeof(buf.f_spare));
 	}
 	if (copy_to_user(p, &buf, sizeof(buf)))
 		return -EFAULT;
@@ -171,6 +171,7 @@ static int do_statfs64(struct kstatfs *st, struct statfs64 __user *p)
 	if (sizeof(buf) == sizeof(*st))
 		memcpy(&buf, st, sizeof(*st));
 	else {
+		memset(&buf, 0, sizeof(buf));
 		buf.f_type = st->f_type;
 		buf.f_bsize = st->f_bsize;
 		buf.f_blocks = st->f_blocks;
@@ -182,7 +183,6 @@ static int do_statfs64(struct kstatfs *st, struct statfs64 __user *p)
 		buf.f_namelen = st->f_namelen;
 		buf.f_frsize = st->f_frsize;
 		buf.f_flags = st->f_flags;
-		memset(buf.f_spare, 0, sizeof(buf.f_spare));
 	}
 	if (copy_to_user(p, &buf, sizeof(buf)))
 		return -EFAULT;
-- 
2.40.1

[PATCH 2/2] s390/uapi: Cover statfs padding by growing f_spare

[Ilya Leoshkevich]

pahole says:

	struct compat_statfs64 {
		...
		u32			f_spare[4];		/*    68    16 */
		/* size: 88, cachelines: 1, members: 12 */
		/* padding: 4 */

	struct statfs {
		...
		unsigned int		f_spare[4];		/*    68    16 */
		/* size: 88, cachelines: 1, members: 12 */
		/* padding: 4 */

	struct statfs64 {
		...
		unsigned int		f_spare[4];		/*    68    16 */
		/* size: 88, cachelines: 1, members: 12 */
		/* padding: 4 */

One has to keep the existence of padding in mind when working with
these structs. Grow f_spare arrays to 5 in order to simplify things.

Acked-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/include/asm/compat.h      | 2 +-
 arch/s390/include/uapi/asm/statfs.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/s390/include/asm/compat.h b/arch/s390/include/asm/compat.h
index a386070f1d56..3cb9d813f022 100644
--- a/arch/s390/include/asm/compat.h
+++ b/arch/s390/include/asm/compat.h
@@ -112,7 +112,7 @@ struct compat_statfs64 {
 	u32		f_namelen;
 	u32		f_frsize;
 	u32		f_flags;
-	u32		f_spare[4];
+	u32		f_spare[5];
 };
 
 /*
diff --git a/arch/s390/include/uapi/asm/statfs.h b/arch/s390/include/uapi/asm/statfs.h
index 72604f7792c3..f85b50723dd3 100644
--- a/arch/s390/include/uapi/asm/statfs.h
+++ b/arch/s390/include/uapi/asm/statfs.h
@@ -30,7 +30,7 @@ struct statfs {
 	unsigned int	f_namelen;
 	unsigned int	f_frsize;
 	unsigned int	f_flags;
-	unsigned int	f_spare[4];
+	unsigned int	f_spare[5];
 };
 
 struct statfs64 {
@@ -45,7 +45,7 @@ struct statfs64 {
 	unsigned int	f_namelen;
 	unsigned int	f_frsize;
 	unsigned int	f_flags;
-	unsigned int	f_spare[4];
+	unsigned int	f_spare[5];
 };
 
 #endif
-- 
2.40.1

Re: [PATCH 0/2] statfs: Enforce statfs[64] structure intialization

[Heiko Carstens]

On Thu, May 04, 2023 at 04:40:19PM +0200, Ilya Leoshkevich wrote:
> This series fixes copying of uninitialized memory to userspace by
> do_statfs_native() and do_statfs64() on s390.
> 
> Patch 1 fixes the problem by making the code similar to
> put_compat_statfs() and put_compat_statfs64().
> 
> Patch 2 gets rid of the padding which caused the issue; even though it
> may be considered redundant, it documents that s390 de-facto has an
> extra f_spare array element.
> 
> Ilya Leoshkevich (2):
>   statfs: Enforce statfs[64] structure intialization
>   s390/uapi: Cover statfs padding by growing f_spare
> 
>  arch/s390/include/asm/compat.h      | 2 +-
>  arch/s390/include/uapi/asm/statfs.h | 4 ++--
>  fs/statfs.c                         | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)

Al, Andrew, should this go via the s390 tree?

Re: [PATCH 1/2] statfs: Enforce statfs[64] structure intialization

[Andrew Morton]

On Thu,  4 May 2023 16:40:20 +0200 Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> s390's struct statfs and struct statfs64 contain padding, which
> field-by-field copying does not set. Initialize the respective structs
> with zeros before filling them and copying them to userspace, like it's
> already done for the compat versions of these structs.
> 
> Found by KMSAN.
> 

Reviewed-by: Andrew Morton <akpm@linux-foundation.org>

Re: [PATCH 0/2] statfs: Enforce statfs[64] structure intialization

[Andrew Morton]

On Thu, 11 May 2023 16:35:15 +0200 Heiko Carstens <hca@linux.ibm.com> wrote:

> On Thu, May 04, 2023 at 04:40:19PM +0200, Ilya Leoshkevich wrote:
> > This series fixes copying of uninitialized memory to userspace by
> > do_statfs_native() and do_statfs64() on s390.
> > 
> > Patch 1 fixes the problem by making the code similar to
> > put_compat_statfs() and put_compat_statfs64().
> > 
> > Patch 2 gets rid of the padding which caused the issue; even though it
> > may be considered redundant, it documents that s390 de-facto has an
> > extra f_spare array element.
> > 
> > Ilya Leoshkevich (2):
> >   statfs: Enforce statfs[64] structure intialization
> >   s390/uapi: Cover statfs padding by growing f_spare
> > 
> >  arch/s390/include/asm/compat.h      | 2 +-
> >  arch/s390/include/uapi/asm/statfs.h | 4 ++--
> >  fs/statfs.c                         | 4 ++--
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> Al, Andrew, should this go via the s390 tree?

I'd say so.

Re: [PATCH 0/2] statfs: Enforce statfs[64] structure intialization

[Alexander Gordeev]

On Thu, May 11, 2023 at 08:45:18PM -0700, Andrew Morton wrote:
> On Thu, 11 May 2023 16:35:15 +0200 Heiko Carstens <hca@linux.ibm.com> wrote:
> > Al, Andrew, should this go via the s390 tree?
> 
> I'd say so.

Hi Al,

Any objections if I pull it via the s390 tree?

Thanks!

Re: [PATCH 0/2] statfs: Enforce statfs[64] structure intialization

[Alexander Gordeev]

On Thu, May 04, 2023 at 04:40:19PM +0200, Ilya Leoshkevich wrote:
> This series fixes copying of uninitialized memory to userspace by
> do_statfs_native() and do_statfs64() on s390.
> 
> Patch 1 fixes the problem by making the code similar to
> put_compat_statfs() and put_compat_statfs64().
> 
> Patch 2 gets rid of the padding which caused the issue; even though it
> may be considered redundant, it documents that s390 de-facto has an
> extra f_spare array element.
> 
> Ilya Leoshkevich (2):
>   statfs: Enforce statfs[64] structure intialization
>   s390/uapi: Cover statfs padding by growing f_spare
> 
>  arch/s390/include/asm/compat.h      | 2 +-
>  arch/s390/include/uapi/asm/statfs.h | 4 ++--
>  fs/statfs.c                         | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)

Series applied,
Thanks!