netdev - RE: [PATCH net-next 5/8] ice: register 1588 PTP clock device object for E810 devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <95887203e7094ffea2306c2130c2b7d6@intel.com>
Date:   Mon, 14 Jun 2021 19:48:28 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Richard Cochran <richardcochran@...il.com>
CC:     "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Brelinski, TonyX" <tonyx.brelinski@...el.com>
Subject: RE: [PATCH net-next 5/8] ice: register 1588 PTP clock device object
 for E810 devices



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Monday, June 14, 2021 11:51 AM
> To: Richard Cochran <richardcochran@...il.com>
> Cc: Keller, Jacob E <jacob.e.keller@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; davem@...emloft.net;
> netdev@...r.kernel.org; sassmann@...hat.com; Brelinski, TonyX
> <tonyx.brelinski@...el.com>
> Subject: Re: [PATCH net-next 5/8] ice: register 1588 PTP clock device object for
> E810 devices
> 
> On Mon, 14 Jun 2021 11:12:20 -0700 Richard Cochran wrote:
> > On Mon, Jun 14, 2021 at 09:43:17AM -0700, Jacob Keller wrote:
> > > > Since dialed_freq is updated regardless of return value of .adjfine
> > > > the driver has no clear way to reject bad scaled_ppm>
> > >
> > > I'm not sure. +Richard?
> >
> > The driver advertises "max_adj".  The PHC layer checks user space inputs:
> >
> > ptp_clock.c line 140:
> > 	} else if (tx->modes & ADJ_FREQUENCY) {
> > 		s32 ppb = scaled_ppm_to_ppb(tx->freq);
> > 		if (ppb > ops->max_adj || ppb < -ops->max_adj)
> > 			return -ERANGE;
> >
> > So, if the max_adj is correct for the driver/HW, then all is well.
> 
> tx->freq is a long, and the conversion to ppb can overflow the s32 type.
> E.g. 281474976645 will become -2 AFAICT. I hacked up phc_ctl to not do
> range checking and kernel happily accepted that value. Shall we do this?
> 
> --->8----
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 03a246e60fd9..ed32fc98d9d8 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -63,7 +63,7 @@ static void enqueue_external_timestamp(struct
> timestamp_event_queue *queue,
>         spin_unlock_irqrestore(&queue->lock, flags);
>  }
> 
> -s32 scaled_ppm_to_ppb(long ppm)
> +s64 scaled_ppm_to_ppb(long ppm)
>  {
>         /*
>          * The 'freq' field in the 'struct timex' is in parts per
> @@ -80,7 +80,7 @@ s32 scaled_ppm_to_ppb(long ppm)
>         s64 ppb = 1 + ppm;
>         ppb *= 125;
>         ppb >>= 13;
> -       return (s32) ppb;
> +       return ppb;
>  }
>  EXPORT_SYMBOL(scaled_ppm_to_ppb);
> 
> @@ -138,7 +138,7 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct
> __kernel_timex *tx)
>                 delta = ktime_to_ns(kt);
>                 err = ops->adjtime(ops, delta);
>         } else if (tx->modes & ADJ_FREQUENCY) {
> -               s32 ppb = scaled_ppm_to_ppb(tx->freq);
> +               s64 ppb = scaled_ppm_to_ppb(tx->freq);
>                 if (ppb > ops->max_adj || ppb < -ops->max_adj)
>                         return -ERANGE;
>                 if (ops->adjfine)

This looks correct to me.