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]

Date:   Mon, 14 Jun 2021 11:08:31 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>,
        Richard Cochran <richardcochran@...il.com>,
        davem@...emloft.net, netdev@...r.kernel.org, sassmann@...hat.com,
        Tony Brelinski <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 09:43:17 -0700 Jacob Keller wrote:
> >> +static int ice_ptp_adjfine(struct ptp_clock_info *info, long scaled_ppm)
> >> +{
> >> +	struct ice_pf *pf = ptp_info_to_pf(info);
> >> +	u64 freq, divisor = 1000000ULL;
> >> +	struct ice_hw *hw = &pf->hw;
> >> +	s64 incval, diff;
> >> +	int neg_adj = 0;
> >> +	int err;
> >> +
> >> +	incval = ICE_PTP_NOMINAL_INCVAL_E810;
> >> +
> >> +	if (scaled_ppm < 0) {
> >> +		neg_adj = 1;
> >> +		scaled_ppm = -scaled_ppm;
> >> +	}
> >> +
> >> +	while ((u64)scaled_ppm > div_u64(U64_MAX, incval)) {
> >> +		/* handle overflow by scaling down the scaled_ppm and
> >> +		 * the divisor, losing some precision
> >> +		 */
> >> +		scaled_ppm >>= 2;
> >> +		divisor >>= 2;
> >> +	}  
> > 
> > I have a question regarding ppm overflows.
> > 
> > We have the max_adj field in struct ptp_clock_info which is checked
> > against ppb, but ppb is a signed 32 bit and scaled_ppm is a long,
> > meaning values larger than S32_MAX << 16 / 1000 will overflow 
> > the ppb calculation, and therefore the check.
> 
> Hmmm.. I thought ppb was a s64, not an s32.
> 
> In general, I believe max_adj is usually capped at 1 billion anyways,
> since it doesn't make sense to slow a clock by more than 1billioln ppb,
> and increasing it more than that isn't really useful either.

Do you mean it's capped somewhere in the code to 1B?

I'm no time expert but this is not probability where 1 is a magic
value, adjusting clock by 1 - 1ppb vs 1 + 1ppb makes little difference,
no? Both mean something is super fishy with the nominal or expected
frequency, but the hardware can do that and more.

Flipping the question, if adjusting by large ppb values is not correct,
why not cap the adjustment at the value which would prevent the u64
overflow?

I don't really have a preferences here, I'm mostly disturbed by 
the overflow in the ppb vs max_adj check.

> > Are we okay with that? Is my math off? Did I miss some part 
> > of the kernel which filters crazy high scaled_ppm/freq?
> > 
> > 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?