On Tue, 2021-03-09 at 10:46 +0100, Philippe Gerum wrote:
> Florian Bezdeka <[email protected]> writes:
>
> > On systems using 32 bit for time_t the sem_timedwait syscall was broken
> > because the function used for copying the timeout value from userspace
> > to kernel (=sem_fetch_timeout()) was always copying
> > sizeof(struct timespec64).
> >
> > A 32 bit application (or more specific an application with 4 byte
> > time_t) would only provide sizeof(struct old_timespec32).
> >
> > Notable changes:
> > - The copy operation from userspace to kernel is now already done in
> > the syscall handler. So it is always done. Previously it was copied
> > over and validated before the first use (when used at all).
> > So we have some additional instructions now that may be
> > unnecessary, but that simplifies the code.
> >
> > - Validation: Switched to timespec64_valid() instead of our own
> > check.
> >
> > Fixes: 8043eccd232d ("cobalt/kernel: y2038: convert struct timespec to
> > timespec64")
> > Signed-off-by: Florian Bezdeka <[email protected]>
> > ---
> > kernel/cobalt/posix/sem.c | 40 +++++++++++++++------------------
> > kernel/cobalt/posix/sem.h | 6 ++---
> > kernel/cobalt/posix/syscall32.c | 10 +++++++--
> > kernel/cobalt/posix/syscall32.h | 2 +-
> > 4 files changed, 29 insertions(+), 29 deletions(-)
> >
> > diff --git a/kernel/cobalt/posix/sem.c b/kernel/cobalt/posix/sem.c
> > index 467a9b7dd..827a4751a 100644
> > --- a/kernel/cobalt/posix/sem.c
> > +++ b/kernel/cobalt/posix/sem.c
> > @@ -267,20 +267,11 @@ out:
> > return ret;
> > }
> >
> >
> >
> >
> > -static inline int sem_fetch_timeout(struct timespec64 *ts,
> > - const void __user *u_ts)
> > -{
> > - return u_ts == NULL ? -EFAULT :
> > - cobalt_copy_from_user(ts, u_ts, sizeof(*ts));
> > -}
> > -
> > int __cobalt_sem_timedwait(struct cobalt_sem_shadow __user *u_sem,
> > - const void __user *u_ts,
> > - int (*fetch_timeout)(struct timespec64 *ts,
> > - const void __user *u_ts))
> > + const struct timespec64 *ts)
> > {
> > - struct timespec64 ts = { .tv_sec = 0, .tv_nsec = 0 };
> > - int pull_ts = 1, ret, info;
> > + int ret, info;
> > + bool validate_ts = true;
> > struct cobalt_sem *sem;
> > xnhandle_t handle;
> > xntmode_t tmode;
> > @@ -304,24 +295,23 @@ int __cobalt_sem_timedwait(struct cobalt_sem_shadow
> > __user *u_sem,
> > * it's actually more complex, to keep some
> > * applications ported to Linux happy.
> > */
> > - if (pull_ts) {
> > + if (validate_ts) {
> > atomic_inc(&sem->state->value);
> > - xnlock_put_irqrestore(&nklock, s);
> > - ret = fetch_timeout(&ts, u_ts);
> > - xnlock_get_irqsave(&nklock, s);
> > - if (ret)
>
> As mentioned in a previous comment on this series, this type of patch is
> subtly changing where the core currently stands with respect to a
> peculiar POSIX compliance issue. Checking the content of a valid
> timespec struct is currently postponed until the timeout is needed, but
> the validity of the timespec pointer referring to that information is
> checked as late as possible too.
I took care of that comment. Please note that "ts" supplied to
__cobalt_sem_timedwait() is non NULL only if it could be properly read.
If the read failed (inside the handler) we will not return an error.
The error will occur when trying to validate later.
>
> If the code now pre-loads the timespec struct early on in the syscall
> path, before the timed services are called, the pointer is explicitly
> checked for validity before we can decide if that timeout information is
> going to be used.
>
> e.g.
>
> struct timespec ts = { .tv_sec = 0, .tv_nsec = 1000000000 };
> sem_init(&sem, 0, 0);
> sem_post(&sem);
> sem_timedwait(&sem, &ts); /* should not fail, and won't as expected. */
This is already part of the smokey y2038 plugin, and does not fail.
>
> but,
>
> sem_init(&sem, 0, 0);
> sem_post(&sem);
> sem_timedwait(&sem, (void *)0xdeadbeefUL); /* should not fail, but will. */
>
This is not yet part of the y2038 smokey tests, but works (= does not
fail). I will add it to the tests as well.
> Since the standard does not mandates such behavior but seems to tag it
> as an implementation-dependent option ("The validity of the abstime need
> not be checked if..."), the change would still be acceptable POSIX-wise
> I believe. However, I'm pretty sure that there are POSIX compliance test
> suites around which would start reporting failures due to this
> change.
>
