Philippe Gerum <[email protected]> writes:
> [email protected] <[email protected]> writes: > >> On Thu, 2021-03-04 at 10:35 +0100, Philippe Gerum wrote: >>> [email protected] <[email protected]> writes: >>> >>> > On Sat, 2021-02-20 at 16:18 +0100, Philippe Gerum via Xenomai wrote: >>> > [snip] >>> > > diff --git a/kernel/cobalt/posix/sem.c b/kernel/cobalt/posix/sem.c >>> > > index 05a861dfe..467a9b7dd 100644 >>> > > --- a/kernel/cobalt/posix/sem.c >>> > > +++ b/kernel/cobalt/posix/sem.c >>> > > @@ -267,7 +267,7 @@ out: >>> > > return ret; >>> > > } >>> > > >>> > > >>> > > >>> > > >>> > > -static inline int sem_fetch_timeout(struct timespec *ts, >>> > > +static inline int sem_fetch_timeout(struct timespec64 *ts, >>> > > const void __user *u_ts) >>> > >>> > Handle the following with care, maybe my understanding of >>> > CONFIG_XENO_ARCH_SYS3264 is wrong. See below... >>> > >>> > My understanding is that this is a breaking change for 32 bit kernels. >>> > The broken part is hidden here, but sem_fetch_timeout() now assumes >>> > that the user provided timespec64, which is not be the case for 32 bit >>> > applications running on a 32 bit kernel. >>> > >>> >>> The user-provided memory is referred to by u_ts, not ts, which receives >>> the timestamp fetch_timeout() is supposed to return. The change should >>> only affect the return type, not the source one. sys32_fetch_timeout() >>> should still do the right thing via sys32_get_timespec(). >> >> Adding the full code: >> >> 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)); >> } >> >> Copying sizeof(*ts) is to much if the application provided >> "__old_time_t" (sizeof(time_t) =4) only. Isn't it? I would expect the >> result (in ts) to be garbage. Due to different padding the "sec" field >> (in ts) would now contain the nsec value from u_ts as well. >> > > Ah, I was looking at the wrong path (32emu). Yes, this is broken for > mere 32bit now (i.e. natively 32bit platform). We could simply use cobalt_get_u_timespec() instead of open coding the call to cobalt_copy_from_user(). Condvars have this right already, mutex, mq and sema4s have their fetch_timeout() helper broken indeed. Good spot. -- Philippe.
