| Message ID | 20240909-listmount_statmount-v4-13-39558204ddf0@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | statmount/listmount testing suites | expand |
Hi! > +static struct statmount *st_mount; > +static struct statmount *st_mount_null; > +static uint64_t mnt_id; > +static uint64_t mnt_id_dont_exist = -1; > +static size_t buff_size; > +static size_t buff_size_invalid = -1; I suppose that we can as well add buff_size_zero, I would expected getting EINVAL from such test. Otherwise: Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Hi! On 10/3/24 16:34, Cyril Hrubis wrote: > Hi! >> +static struct statmount *st_mount; >> +static struct statmount *st_mount_null; >> +static uint64_t mnt_id; >> +static uint64_t mnt_id_dont_exist = -1; >> +static size_t buff_size; >> +static size_t buff_size_invalid = -1; > I suppose that we can as well add buff_size_zero, I would expected > getting EINVAL from such test. That seems to be a valid value, since I get PASS. Kernel bug? > Otherwise: > > Reviewed-by: Cyril Hrubis <chrubis@suse.cz> > Andrea
Hi!
> That seems to be a valid value, since I get PASS. Kernel bug?
Looking at kernel it's a bit confusing, the bufsize is supposed to be
the size of the structure plus the buffer allocated for the string
reply. Which makes the code more complicated than it could have been if
these two were separeted properly.
However we do have:
if (kbufsize >= s->bufsize)
return -EOVERFLOW;
in statmount_string(), so we will trigger the error there if we pass one
of the requests that is supposed to produce a string reply.
But there seems to be something strange going on in the
prepare_kstatmount() as well, we do have:
if (ks->mask & STATMOUNT_STRING_REQ) {
if (bufsize == sizeof(ks->sm))
return -EOVERFLOW;
^ This line should probably be if (bufsize <= sizeof(ks->sm))
because we are trying to make sure that the structure is large
enough that we can write at the offset where the
string area starts.
It seems to be guarded by the checks in
statmount_string() later on as well, so I suppose that we
will trigger the EOVERFLOW slightly later if this does not
work though. CCying Jan to have a look.
ks->seq.buf = kvmalloc(seq_size, GFP_KERNEL_ACCOUNT);
if (!ks->seq.buf)
return -ENOMEM;
ks->seq.size = seq_size;
}
However the size seems to be properly used in the
copy_statmount_to_user()
size_t copysize = min_t(size_t, s->bufsize, sizeof(*sm));
...
if (copy_to_user(s->buf, sm, copysize))
return -EFAULT;
This means that we will copy as much of the statmount structure to
userspace as we requested, so in the case that we pass 0 as bufsize none
of the data should be copied and indeed we should get back success.
Thanks for the explanation! I will fix the other patches and send the new version. Andrea On 10/4/24 11:17, Cyril Hrubis wrote: > Hi! >> That seems to be a valid value, since I get PASS. Kernel bug? > Looking at kernel it's a bit confusing, the bufsize is supposed to be > the size of the structure plus the buffer allocated for the string > reply. Which makes the code more complicated than it could have been if > these two were separeted properly. > > > However we do have: > > if (kbufsize >= s->bufsize) > return -EOVERFLOW; > > in statmount_string(), so we will trigger the error there if we pass one > of the requests that is supposed to produce a string reply. > > > But there seems to be something strange going on in the > prepare_kstatmount() as well, we do have: > > if (ks->mask & STATMOUNT_STRING_REQ) { > if (bufsize == sizeof(ks->sm)) > return -EOVERFLOW; > > ^ This line should probably be if (bufsize <= sizeof(ks->sm)) > because we are trying to make sure that the structure is large > enough that we can write at the offset where the > string area starts. > > It seems to be guarded by the checks in > statmount_string() later on as well, so I suppose that we > will trigger the EOVERFLOW slightly later if this does not > work though. CCying Jan to have a look. > > ks->seq.buf = kvmalloc(seq_size, GFP_KERNEL_ACCOUNT); > if (!ks->seq.buf) > return -ENOMEM; > > ks->seq.size = seq_size; > } > > > > > However the size seems to be properly used in the > copy_statmount_to_user() > > size_t copysize = min_t(size_t, s->bufsize, sizeof(*sm)); > > ... > > if (copy_to_user(s->buf, sm, copysize)) > return -EFAULT; > > > This means that we will copy as much of the statmount structure to > userspace as we requested, so in the case that we pass 0 as bufsize none > of the data should be copied and indeed we should get back success. >
Hi! On Fri 04-10-24 11:17:21, Cyril Hrubis wrote: > > That seems to be a valid value, since I get PASS. Kernel bug? > > Looking at kernel it's a bit confusing, the bufsize is supposed to be > the size of the structure plus the buffer allocated for the string > reply. Which makes the code more complicated than it could have been if > these two were separeted properly. > > > However we do have: > > if (kbufsize >= s->bufsize) > return -EOVERFLOW; > > in statmount_string(), so we will trigger the error there if we pass one > of the requests that is supposed to produce a string reply. Yes, if STATMOUNT_STRING_REQ is set, the above check should always make sure bufsize is large enough. > But there seems to be something strange going on in the > prepare_kstatmount() as well, we do have: > > if (ks->mask & STATMOUNT_STRING_REQ) { > if (bufsize == sizeof(ks->sm)) > return -EOVERFLOW; > > ^ This line should probably be if (bufsize <= sizeof(ks->sm)) > because we are trying to make sure that the structure is large > enough that we can write at the offset where the > string area starts. > > It seems to be guarded by the checks in > statmount_string() later on as well, so I suppose that we > will trigger the EOVERFLOW slightly later if this does not > work though. CCying Jan to have a look. Yup, this looks confusing to me as well. Christian, is this check indeed meant to bail early if the buffer obviously is not large enough? Shouldn't it then be <= as Cyril suggests? I have a feeling I might be missing some "extensible syscall" magic that does the check bufsize < sizeof(struct) automatically somewhere but I could not find it... Honza
diff --git a/runtest/syscalls b/runtest/syscalls index 209e04b79..125d8fb00 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1574,6 +1574,7 @@ statmount03 statmount03 statmount04 statmount04 statmount05 statmount05 statmount06 statmount06 +statmount07 statmount07 statfs01 statfs01 statfs01_64 statfs01_64 diff --git a/testcases/kernel/syscalls/statmount/.gitignore b/testcases/kernel/syscalls/statmount/.gitignore index 03a75bd40..b2a55c077 100644 --- a/testcases/kernel/syscalls/statmount/.gitignore +++ b/testcases/kernel/syscalls/statmount/.gitignore @@ -4,3 +4,4 @@ statmount03 statmount04 statmount05 statmount06 +statmount07 diff --git a/testcases/kernel/syscalls/statmount/statmount07.c b/testcases/kernel/syscalls/statmount/statmount07.c new file mode 100644 index 000000000..2746bf424 --- /dev/null +++ b/testcases/kernel/syscalls/statmount/statmount07.c @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com> + */ + +/*\ + * [Description] + * + * This test verifies that statmount() is raising the correct errors according + * with invalid input values. + */ + +#define _GNU_SOURCE + +#include "statmount.h" +#include "lapi/stat.h" + +#define MNTPOINT "mntpoint" + +static struct statmount *st_mount; +static struct statmount *st_mount_null; +static uint64_t mnt_id; +static uint64_t mnt_id_dont_exist = -1; +static size_t buff_size; +static size_t buff_size_invalid = -1; + +struct tcase { + int exp_errno; + char *msg; + uint64_t *mnt_id; + uint64_t mask; + unsigned int flags; + size_t *buff_size; + struct statmount **buff; +} tcases[] = { + {ENOENT, "mount id doesn't exist'", &mnt_id_dont_exist, 0, 0, &buff_size, &st_mount}, + {EOVERFLOW, "invalid mask", &mnt_id, -1, 0, &buff_size, &st_mount}, + {EINVAL, "flags must be zero", &mnt_id, 0, 1, &buff_size, &st_mount}, + {EFAULT, "invalid buffer size", &mnt_id, 0, 0, &buff_size_invalid, &st_mount}, + {EFAULT, "invalid buffer pointer", &mnt_id, 0, 0, &buff_size, &st_mount_null}, +}; + +static void run(unsigned int n) +{ + struct tcase *tc = &tcases[n]; + + memset(st_mount, 0, sizeof(struct statmount)); + + TST_EXP_FAIL(statmount(*tc->mnt_id, tc->mask, + *tc->buff, *tc->buff_size, tc->flags), + tc->exp_errno, "%s", tc->msg); +} + +static void setup(void) +{ + struct ltp_statx sx; + + SAFE_STATX(AT_FDCWD, MNTPOINT, 0, STATX_MNT_ID_UNIQUE, &sx); + + mnt_id = sx.data.stx_mnt_id; + buff_size = sizeof(struct statmount); +} +static struct tst_test test = { + .test = run, + .tcnt = ARRAY_SIZE(tcases), + .setup = setup, + .min_kver = "6.8", + .mount_device = 1, + .mntpoint = MNTPOINT, + .format_device = 1, + .bufs = (struct tst_buffers []) { + {&st_mount, .size = sizeof(struct statmount)}, + {} + } +};