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: <20210614115043.4e2b48da@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 14 Jun 2021 11:50:43 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     Jacob Keller <jacob.e.keller@...el.com>,
        Tony Nguyen <anthony.l.nguyen@...el.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 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)