Re: [PATCH 01/13] y2038: cobalt: Introduce some itimerspec64 related helpers

[Jan Kiszka <jan.kiszka@siemens.com>]

On 15.05.23 11:31, Florian Bezdeka wrote:
> On Fri, 2023-05-12 at 17:59 +0200, Jan Kiszka wrote:
>> On 08.05.23 10:13, Florian Bezdeka wrote:
>>> The introduced helpers will be used by the timer and timerfd y2038
>>> related services for reading/writing itimerspec from/to userspace.
>>>
>>> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
>>> ---
>>>  include/cobalt/kernel/time.h | 21 +++++++++++++++++++++
>>>  kernel/cobalt/time.c         | 38 ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 59 insertions(+)
>>>
>>> diff --git a/include/cobalt/kernel/time.h b/include/cobalt/kernel/time.h
>>> index a55398068..e348cf9b8 100644
>>> --- a/include/cobalt/kernel/time.h
>>> +++ b/include/cobalt/kernel/time.h
>>> @@ -28,4 +28,25 @@ int cobalt_get_timespec64(struct timespec64 *ts,
>>>  int cobalt_put_timespec64(const struct timespec64 *ts,
>>>  			  struct __kernel_timespec __user *uts);
>>>  
>>> +/**
>>> + * Read struct __kernel_itimerspec from userspace and convert to
>>> + * struct itimerspec64
>>> + *
>>> + * @param dst The destination, will be filled
>>> + * @param src The source, provided by an application
>>> + * @return 0 on success, -EFAULT otherwise
>>> + */
>>> +int cobalt_get_itimerspec64(struct itimerspec64 *dst,
>>> +			    const struct __kernel_itimerspec __user *src);
>>> +
>>> +/**
>>> + * Convert struct itimerspec64 to struct __kernel_itimerspec and copy to user
>>> + * space
>>> + * @param dst The destination, will be filled, provided by an application
>>> + * @param src The source, provided by the kernel
>>> + * @return 0 un success, -EFAULT otherwise
>>> + */
>>> +int cobalt_put_itimerspec64(struct __kernel_itimerspec __user *dst,
>>> +			    const struct itimerspec64 *src);
>>> +
>>>  #endif //_COBALT_KERNEL_TIME_H
>>> diff --git a/kernel/cobalt/time.c b/kernel/cobalt/time.c
>>> index 27dbf8290..716223dc5 100644
>>> --- a/kernel/cobalt/time.c
>>> +++ b/kernel/cobalt/time.c
>>> @@ -36,3 +36,41 @@ int cobalt_put_timespec64(const struct timespec64 *ts,
>>>  
>>>  	return cobalt_copy_to_user(uts, &kts, sizeof(kts)) ? -EFAULT : 0;
>>>  }
>>> +
>>> +int cobalt_get_itimerspec64(struct itimerspec64 *dst,
>>> +			    const struct __kernel_itimerspec __user *src)
>>> +{
>>> +	struct timespec64 interval, value;
>>> +	int ret;
>>> +
>>> +	if (!src)
>>
>> Can that be enough to validate the pointer? Or is it even needed? We
>> must validate it via cobalt_get_timespec64 anyway, no?
> 
> I think we could remove this check but it improves the readability (and
> code flow) a lot. 
> 
> Without this check cobalt_get_timespec64 (called below) would trigger a
> fault while reading from this address when src is NULL. (&src->it_* is
> a low offset). The result is basically the same but we would migrate to
> seconary domain first, handle the fault there and then exit to
> userspace. No?

&NULL->offset is just a pointer, not a dereference. And even if we check
for NULL, NULL+1 would still pass.

Jan

> 
> Florian
> 
>>
>>> +		return -EFAULT;
>>> +
>>> +	ret = cobalt_get_timespec64(&interval, &src->it_interval);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = cobalt_get_timespec64(&value, &src->it_value);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	dst->it_interval.tv_sec = interval.tv_sec;
>>> +	dst->it_interval.tv_nsec = interval.tv_nsec;
>>> +	dst->it_value.tv_sec = value.tv_sec;
>>> +	dst->it_value.tv_nsec = value.tv_nsec;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int cobalt_put_itimerspec64(struct __kernel_itimerspec __user *dst,
>>> +			    const struct itimerspec64 *src)
>>> +{
>>> +	struct __kernel_itimerspec kits = {
>>> +		.it_interval.tv_sec = src->it_interval.tv_sec,
>>> +		.it_interval.tv_nsec = src->it_interval.tv_nsec,
>>> +		.it_value.tv_sec = src->it_value.tv_sec,
>>> +		.it_value.tv_nsec = src->it_value.tv_nsec
>>> +	};
>>> +
>>> +	return cobalt_copy_to_user(dst, &kits, sizeof(kits));
>>> +}
>>>
>>
>> Jan
>>
> 

-- 
Siemens AG, Technology
Competence Center Embedded Linux