On 04.03.21 11:08, Philippe Gerum wrote: > > 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. >
Oops, missed this. Patch is in next (all 5, in fact, and all built find), but I will revert things back to patch 1 only. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
