| Message ID | 20240906034806.1161083-2-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: Check timespec64 before call settime64() | expand |
On Fri, Sep 06, 2024 at 11:48:05AM +0800, Jinjie Ruan wrote:
> As Andrew pointed out, it will make sence that the PTP core
s/sence/sense/
Thanks,
Richard
On Fri, Sep 06, 2024 at 11:48:05AM +0800, Jinjie Ruan wrote: > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index c56cd0f63909..cf75899a6681 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c > @@ -100,6 +100,16 @@ static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp > return -EBUSY; > } > > + if (!tp) { > + pr_warn("ptp: tp == NULL\n"); > + return -EINVAL; > + } This check is pointless because `tp` cannot be null. See SYSCALL_DEFINE2(clock_settime, ...) > + if (!timespec64_valid(tp)) { > + pr_warn("ptp: tv_sec or tv_usec out of range\n"); > + return -ERANGE; > + } Shouldn't this be done at the higher layer, in clock_settime() ? Thanks, Richard
On 2024/9/6 12:27, Richard Cochran wrote: > On Fri, Sep 06, 2024 at 11:48:05AM +0800, Jinjie Ruan wrote: > >> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c >> index c56cd0f63909..cf75899a6681 100644 >> --- a/drivers/ptp/ptp_clock.c >> +++ b/drivers/ptp/ptp_clock.c >> @@ -100,6 +100,16 @@ static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp >> return -EBUSY; >> } >> >> + if (!tp) { >> + pr_warn("ptp: tp == NULL\n"); >> + return -EINVAL; >> + } > > This check is pointless because `tp` cannot be null. Yes, this one is unnecessary and it is also unnecessary in the lan743x_ptpci_settime64(). > > See SYSCALL_DEFINE2(clock_settime, ...) > >> + if (!timespec64_valid(tp)) { >> + pr_warn("ptp: tv_sec or tv_usec out of range\n"); >> + return -ERANGE; >> + } > > Shouldn't this be done at the higher layer, in clock_settime() ? Maybe it is more reasonable? > > Thanks, > Richard
On Fri, Sep 06, 2024 at 11:48:05AM +0800, Jinjie Ruan wrote: > As Andrew pointed out, it will make sence that the PTP core > checked timespec64 struct's tv_sec and tv_nsec range before calling > ptp->info->settime64(), so check it ahead. > > There are some drivers that use tp->tv_sec and tp->tv_nsec directly to > write registers without validity checks and assume that the PTP core has > been checked, 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. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Suggested-by: Andrew Lunn <andrew@lunn.ch> FYI: Your Signed-off-by: should be last. Please fix this when you respin as requested by Richard. Andrew --- pw-bot: cr
On Fri, Sep 06, 2024 at 02:37:58PM +0800, Jinjie Ruan wrote: > > See SYSCALL_DEFINE2(clock_settime, ...) > > > >> + if (!timespec64_valid(tp)) { > >> + pr_warn("ptp: tv_sec or tv_usec out of range\n"); > >> + return -ERANGE; > >> + } > > > > Shouldn't this be done at the higher layer, in clock_settime() ? > > Maybe it is more reasonable? I think so. If you code that up, please include lkml and the time keeping folks (Miroslav, John Stultz, tglx) on CC. Thanks, Richard
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index c56cd0f63909..cf75899a6681 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -100,6 +100,16 @@ static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp return -EBUSY; } + if (!tp) { + pr_warn("ptp: tp == NULL\n"); + return -EINVAL; + } + + if (!timespec64_valid(tp)) { + pr_warn("ptp: tv_sec or tv_usec out of range\n"); + return -ERANGE; + } + return ptp->info->settime64(ptp->info, tp); }
As Andrew pointed out, it will make sence that the PTP core checked timespec64 struct's tv_sec and tv_nsec range before calling ptp->info->settime64(), so check it ahead. There are some drivers that use tp->tv_sec and tp->tv_nsec directly to write registers without validity checks and assume that the PTP core has been checked, 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. Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> Suggested-by: Andrew Lunn <andrew@lunn.ch> --- drivers/ptp/ptp_clock.c | 10 ++++++++++ 1 file changed, 10 insertions(+)