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
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 <[email protected]>
Cc: [email protected] # v4.14+
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
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
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?
On Thu, 11 May 2023 16:35:15 +0200 Heiko Carstens <[email protected]> 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.
On Thu, 4 May 2023 16:40:20 +0200 Ilya Leoshkevich <[email protected]> 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 <[email protected]>
On Thu, May 11, 2023 at 08:45:18PM -0700, Andrew Morton wrote:
> On Thu, 11 May 2023 16:35:15 +0200 Heiko Carstens <[email protected]> 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!
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!