Re: [PATCH v3 0/7] Isolate time_t data types for clock/timer syscalls

[Arnd Bergmann]

On Mon, Jun 26, 2017 at 8:17 PM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
> On Sun, Jun 25, 2017 at 7:35 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>> On Sat, Jun 24, 2017 at 11:45:01AM -0700, Deepa Dinamani wrote:
>>> The series aims at isolating data conversions of time_t based structures:
>>> struct timespec and struct itimerspec at user space boundaries.
>>> This helps to later change the underlying types to handle y2038 changes
>>> to these.
>>
>> Nice...  A few questions:
>>
>> * what about setitimer(2)?  Right now that's the only remaining user of
>> get_compat_itimerval(); similar for getitimer(2) and put_compat_itimerval().
>
> We do not plan to support these beyond y2038 on 32 bit systems.
> timer_settime() and timer_gettime() are considered to be replacements
> for these, respectively.
>
> There is also going to be a cleanup of timeval/ timespec/ time_t data
> types and apis after the new syscalls are ready.
> At that time I might choose to get rid of these itimerval apis. I'm
> not sure yet.

I see that internally, alarm/getitimer/setitimer all use ktime_t, so
one possible solution would be to push down the use of ktime_t
into the callers and do both the conversion and range check in the
user copy function.

>> * you have two callers of get_compat_itimerspec64(); one is followed by
>> itimerspec64_valid(), another - by its open-coded analogue.  The same
>> goes for get_itimerspec64(); wouldn't it be better to have both check
>> the validity immediately and simply fail with -EINVAL?  Matter of taste,
>> but...
>
> This is what I thought also. And, in fact this is how I had it in one
> of the earlier version of my series.
> But, the utimensat(2) is what I consider provides a counter example of
> why this is a bad idea.
> There is no reason you should not be able to read the itimerspec64 and
> not check for validity soon after. Meaning there can be special
> markers like UTIME_NOW and UTIME_OMIT in the nanosecond field which
> will make the validity check fail. But, is perfectly normal for the
> syscall under consideration.

If this is the only case we find, we could create another get_utimes_arg()
(and compat_get_utimes_arg()) helper just for this one, which then copies
both timespec structures at once and does the appropriate checks.

Another problem might be interfaces that don't have a range check today
with existing users that may pass invalid data and expect it to succeed
(or fail in a particular way). We probably want to add range checks here
in most cases, but having the option of not doing the checking in some
cases may be useful. Maybe I'm just overly cautious here though.

         Arnd