| Message ID | 20240909074124.964907-2-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | posix-timers: Check timespec64 before call clock_set() | expand |
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()? 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
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
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.
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(+)