As Andrew pointed out, it will make sense that the PTP core
checked timespec64 struct's tv_sec and tv_nsec range before calling
ptp->info->settime64(). Check it ahead in more higher layer
clock_settime() as Richard suggested.

There are some drivers that use tp->tv_sec and tp->tv_nsec directly to
write registers without validity checks and assume that the higher layer
has checked it, which is dangerous and will benefit from this, such as
hclge_ptp_settime(), igb_ptp_settime_i210(), _rcar_gen4_ptp_settime(),
and some drivers can remove the checks of itself.

Suggested-by: Richard Cochran <richardcochran@gmail.com>
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v3:
- Adjust to check in more higher layer clock_settime().
- Remove the NULL check.
- Update the commit message and subject.
v2:
- Adjust to check in ptp_clock_settime().
---
 kernel/time/posix-timers.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 1cc830ef93a7..34deec619e17 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
 	if (get_timespec64(&new_tp, tp))
 		return -EFAULT;
 
+	if (!timespec64_valid(&new_tp))
+		return -ERANGE;
+
 	/*
 	 * Permission checks have to be done inside the clock specific
 	 * setter callback.
-- 
2.34.1

On Mon, Sep 09, 2024 at 03:41:23PM +0800, Jinjie Ruan wrote:
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 1cc830ef93a7..34deec619e17 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
>  	if (get_timespec64(&new_tp, tp))
>  		return -EFAULT;
>  
> +	if (!timespec64_valid(&new_tp))
> +		return -ERANGE;

Why not use timespec64_valid_settod()?

Thanks,
Richard

On 2024/9/9 23:19, Richard Cochran wrote:
> On Mon, Sep 09, 2024 at 03:41:23PM +0800, Jinjie Ruan wrote:
>> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
>> index 1cc830ef93a7..34deec619e17 100644
>> --- a/kernel/time/posix-timers.c
>> +++ b/kernel/time/posix-timers.c
>> @@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
>>  	if (get_timespec64(&new_tp, tp))
>>  		return -EFAULT;
>>  
>> +	if (!timespec64_valid(&new_tp))
>> +		return -ERANGE;
> 
> Why not use timespec64_valid_settod()?

There was already checks in following code, so it is not necessary to
check NULL or timespec64_valid() in ptp core and its drivers, only the
second patch is needed.

169 int do_sys_settimeofday64(const struct timespec64 *tv, const struct
timezone *tz)
 170 {
 171 >-------static int firsttime = 1;
 172 >-------int error = 0;
 173
 174 >-------if (tv && !timespec64_valid_settod(tv))
 175 >------->-------return -EINVAL;



> 
> Thanks,
> Richard

On Thu, Sep 12 2024 at 10:53, Jinjie Ruan wrote:

> On 2024/9/9 23:19, Richard Cochran wrote:
>> On Mon, Sep 09, 2024 at 03:41:23PM +0800, Jinjie Ruan wrote:
>>> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
>>> index 1cc830ef93a7..34deec619e17 100644
>>> --- a/kernel/time/posix-timers.c
>>> +++ b/kernel/time/posix-timers.c
>>> @@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
>>>  	if (get_timespec64(&new_tp, tp))
>>>  		return -EFAULT;
>>>  
>>> +	if (!timespec64_valid(&new_tp))
>>> +		return -ERANGE;
>> 
>> Why not use timespec64_valid_settod()?
>
> There was already checks in following code, so it is not necessary to
> check NULL or timespec64_valid() in ptp core and its drivers, only the
> second patch is needed.
>
> 169 int do_sys_settimeofday64(const struct timespec64 *tv, const struct
> timezone *tz)
>  170 {
>  171 >-------static int firsttime = 1;
>  172 >-------int error = 0;
>  173
>  174 >-------if (tv && !timespec64_valid_settod(tv))
>  175 >------->-------return -EINVAL;

How does this code validate timespecs for clock_settime(clockid) where
clockid != CLOCK_REALTIME?

Thanks,

        tglx

On 2024/9/12 20:04, Thomas Gleixner wrote:
> On Thu, Sep 12 2024 at 10:53, Jinjie Ruan wrote:
> 
>> On 2024/9/9 23:19, Richard Cochran wrote:
>>> On Mon, Sep 09, 2024 at 03:41:23PM +0800, Jinjie Ruan wrote:
>>>> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
>>>> index 1cc830ef93a7..34deec619e17 100644
>>>> --- a/kernel/time/posix-timers.c
>>>> +++ b/kernel/time/posix-timers.c
>>>> @@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
>>>>  	if (get_timespec64(&new_tp, tp))
>>>>  		return -EFAULT;
>>>>  
>>>> +	if (!timespec64_valid(&new_tp))
>>>> +		return -ERANGE;
>>>
>>> Why not use timespec64_valid_settod()?
>>
>> There was already checks in following code, so it is not necessary to
>> check NULL or timespec64_valid() in ptp core and its drivers, only the
>> second patch is needed.
>>
>> 169 int do_sys_settimeofday64(const struct timespec64 *tv, const struct
>> timezone *tz)
>>  170 {
>>  171 >-------static int firsttime = 1;
>>  172 >-------int error = 0;
>>  173
>>  174 >-------if (tv && !timespec64_valid_settod(tv))
>>  175 >------->-------return -EINVAL;
> 
> How does this code validate timespecs for clock_settime(clockid) where
> clockid != CLOCK_REALTIME?

According to the man manual of clock_settime(), the other clockids are
not settable.

And in Linux kernel code, except for CLOCK_REALTIME which is defined in
posix_clocks array, the clock_set() hooks are not defined and will
return -EINVAL in SYSCALL_DEFINE2(clock_settime), so the check is not
necessary.

> 
> Thanks,
> 
>         tglx

On Thu, Sep 12 2024 at 20:24, Jinjie Ruan wrote:
> On 2024/9/12 20:04, Thomas Gleixner wrote:
>> How does this code validate timespecs for clock_settime(clockid) where
>> clockid != CLOCK_REALTIME?
>
> According to the man manual of clock_settime(), the other clockids are
> not settable.
>
> And in Linux kernel code, except for CLOCK_REALTIME which is defined in
> posix_clocks array, the clock_set() hooks are not defined and will
> return -EINVAL in SYSCALL_DEFINE2(clock_settime), so the check is not
> necessary.

You clearly understand the code you are modifying:

const struct k_clock clock_posix_dynamic = {
	.clock_getres           = pc_clock_getres,
        .clock_set              = pc_clock_settime, 

which is what PTP clocks use and that's what this is about, no?

Thanks,

        tglx

On 2024/9/13 18:46, Thomas Gleixner wrote:
> On Thu, Sep 12 2024 at 20:24, Jinjie Ruan wrote:
>> On 2024/9/12 20:04, Thomas Gleixner wrote:
>>> How does this code validate timespecs for clock_settime(clockid) where
>>> clockid != CLOCK_REALTIME?
>>
>> According to the man manual of clock_settime(), the other clockids are
>> not settable.
>>
>> And in Linux kernel code, except for CLOCK_REALTIME which is defined in
>> posix_clocks array, the clock_set() hooks are not defined and will
>> return -EINVAL in SYSCALL_DEFINE2(clock_settime), so the check is not
>> necessary.
> 
> You clearly understand the code you are modifying:
> 
> const struct k_clock clock_posix_dynamic = {
> 	.clock_getres           = pc_clock_getres,
>         .clock_set              = pc_clock_settime, 
> 
> which is what PTP clocks use and that's what this is about, no?

Yes, it uses the dynamic one rather than the static ones.

> 
> Thanks,
> 
>         tglx

On 2024/9/9 23:19, Richard Cochran wrote:
> On Mon, Sep 09, 2024 at 03:41:23PM +0800, Jinjie Ruan wrote:
>> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
>> index 1cc830ef93a7..34deec619e17 100644
>> --- a/kernel/time/posix-timers.c
>> +++ b/kernel/time/posix-timers.c
>> @@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
>>  	if (get_timespec64(&new_tp, tp))
>>  		return -EFAULT;
>>  
>> +	if (!timespec64_valid(&new_tp))
>> +		return -ERANGE;
> 
> Why not use timespec64_valid_settod()?

It seems more limited and is only used in timekeeping or
do_sys_settimeofday64().

And the timespec64_valid() is looser and wider used, which I think is
more appropriate here.

> 
> Thanks,
> Richard

On Tue, Sep 10 2024 at 19:23, Jinjie Ruan wrote:
> On 2024/9/9 23:19, Richard Cochran wrote:
>> On Mon, Sep 09, 2024 at 03:41:23PM +0800, Jinjie Ruan wrote:
>>> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
>>> index 1cc830ef93a7..34deec619e17 100644
>>> --- a/kernel/time/posix-timers.c
>>> +++ b/kernel/time/posix-timers.c
>>> @@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
>>>  	if (get_timespec64(&new_tp, tp))
>>>  		return -EFAULT;
>>>  
>>> +	if (!timespec64_valid(&new_tp))
>>> +		return -ERANGE;
>> 
>> Why not use timespec64_valid_settod()?
>
> It seems more limited and is only used in timekeeping or
> do_sys_settimeofday64().

For a very good reason.

> And the timespec64_valid() is looser and wider used, which I think is
> more appropriate here.

Can you please stop this handwaving and provide proper technical
arguments?

Why would PTP have less strict requirements than settimeofday()?

Thanks,

        tglx

On 2024/9/10 20:05, Thomas Gleixner wrote:
> On Tue, Sep 10 2024 at 19:23, Jinjie Ruan wrote:
>> On 2024/9/9 23:19, Richard Cochran wrote:
>>> On Mon, Sep 09, 2024 at 03:41:23PM +0800, Jinjie Ruan wrote:
>>>> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
>>>> index 1cc830ef93a7..34deec619e17 100644
>>>> --- a/kernel/time/posix-timers.c
>>>> +++ b/kernel/time/posix-timers.c
>>>> @@ -1137,6 +1137,9 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
>>>>  	if (get_timespec64(&new_tp, tp))
>>>>  		return -EFAULT;
>>>>  
>>>> +	if (!timespec64_valid(&new_tp))
>>>> +		return -ERANGE;
>>>
>>> Why not use timespec64_valid_settod()?
>>
>> It seems more limited and is only used in timekeeping or
>> do_sys_settimeofday64().
> 
> For a very good reason.
> 
>> And the timespec64_valid() is looser and wider used, which I think is
>> more appropriate here.
> 
> Can you please stop this handwaving and provide proper technical
> arguments?
> 
> Why would PTP have less strict requirements than settimeofday()?

I checked all the PTP driver, most of them use timespec64_to_ns()
convert them to ns which already have a check, but the others not check
them, and lan743x_ptp check them differently and more, so i think this
is a minimum check.

Use timespec64_to_ns()
- drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
- drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:
- drivers/net/ethernet/cavium/liquidio/lio_main.c:
- drivers/net/ethernet/engleder/tsnep_ptp.c
- drivers/net/ethernet/freescale/fec_ptp.c
- drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c:
- drivers/net/ethernet/intel/i40e/i40e_ptp.c
- drivers/net/ethernet/intel/ice/ice_ptp.c
- drivers/net/ethernet/intel/igc/igc_ptp.c
- drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
- drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c
- drivers/net/ethernet/qlogic/qede/qede_ptp.c
- drivers/ptp/ptp_idt82p33.c

Not check:
- drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
- drivers/net/ethernet/intel/igb/igb_ptp.c (only one igb_ptp_settime_i210())
- drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
- drivers/net/ethernet/renesas/rcar_gen4_ptp.c
-
drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
- drivers/net/phy/micrel.c
- drivers/ptp/ptp_dfl_tod.c

Self check and check more:
- drivers/net/ethernet/microchip/lan743x_ptp.c

> 
> Thanks,
> 
>         tglx
> 
>

On Tue, Sep 10 2024 at 20:30, Jinjie Ruan wrote:
> On 2024/9/10 20:05, Thomas Gleixner wrote:
>> Can you please stop this handwaving and provide proper technical
>> arguments?
>> 
>> Why would PTP have less strict requirements than settimeofday()?
>
> I checked all the PTP driver, most of them use timespec64_to_ns()
> convert them to ns which already have a check, but the others not check
> them, and lan743x_ptp check them differently and more, so i think this
> is a minimum check.

It does not matter at all what the PTP drivers do. What matters is what
is correct and what not.

What they do is actually wrong as they simply cut off an overly large
value instead of rejecting it in the first place. That's not a check at
all.

The cutoff in timespec64_to_ns() is there to saturate the result instead
of running into a multiplication overflow. That's correct for some use
cases, but not a replacement for an actual useful range check.

This is about correctness and correctness is not defined by what a bunch
of drivers implement which are all a big copy & pasta orgy.

Thanks,

        tglx