[PATCH net] ptp: improve max_adj check against unreasonable values

[PATCH net] ptp: improve max_adj check against unreasonable values

[Jakub Kicinski]

Scaled PPM conversion to PPB may (on 64bit systems) result
in a value larger than s32 can hold (freq/scaled_ppm is a long).
This means the kernel will not correctly reject unreasonably
high ->freq values (e.g. > 4294967295ppb, 281474976645 scaled PPM).

The conversion is equivalent to a division by ~66 (65.536),
so the value of ppb is always smaller than ppm, but not small
enough to assume narrowing the type from long -> s32 is okay.

Note that reasonable user space (e.g. ptp4l) will not use such
high values, anyway, 4289046510ppb ~= 4.3x, so the fix is
somewhat pedantic.

Fixes: d39a743511cd ("ptp: validate the requested frequency adjustment.")
Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/ptp/ptp_clock.c          | 6 +++---
 include/linux/ptp_clock_kernel.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 03a246e60fd9..21c4c34c52d8 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)
+long 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 (long) 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);
+		long ppb = scaled_ppm_to_ppb(tx->freq);
 		if (ppb > ops->max_adj || ppb < -ops->max_adj)
 			return -ERANGE;
 		if (ops->adjfine)
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 0d47fd33b228..51d7f1b8b32a 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -235,7 +235,7 @@ extern int ptp_clock_index(struct ptp_clock *ptp);
  * @ppm:    Parts per million, but with a 16 bit binary fractional field
  */
 
-extern s32 scaled_ppm_to_ppb(long ppm);
+extern long scaled_ppm_to_ppb(long ppm);
 
 /**
  * ptp_find_pin() - obtain the pin index of a given auxiliary function
-- 
2.31.1

Re: [PATCH net] ptp: improve max_adj check against unreasonable values

[Richard Cochran]

On Mon, Jun 14, 2021 at 03:24:05PM -0700, Jakub Kicinski wrote:
> Scaled PPM conversion to PPB may (on 64bit systems) result
> in a value larger than s32 can hold (freq/scaled_ppm is a long).
> This means the kernel will not correctly reject unreasonably
> high ->freq values (e.g. > 4294967295ppb, 281474976645 scaled PPM).
> 
> The conversion is equivalent to a division by ~66 (65.536),
> so the value of ppb is always smaller than ppm, but not small
> enough to assume narrowing the type from long -> s32 is okay.
> 
> Note that reasonable user space (e.g. ptp4l) will not use such
> high values, anyway, 4289046510ppb ~= 4.3x, so the fix is
> somewhat pedantic.

But still important to defend against fuzzing!
 
> Fixes: d39a743511cd ("ptp: validate the requested frequency adjustment.")
> Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH net] ptp: improve max_adj check against unreasonable values

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Mon, 14 Jun 2021 15:24:05 -0700 you wrote:
> Scaled PPM conversion to PPB may (on 64bit systems) result
> in a value larger than s32 can hold (freq/scaled_ppm is a long).
> This means the kernel will not correctly reject unreasonably
> high ->freq values (e.g. > 4294967295ppb, 281474976645 scaled PPM).
> 
> The conversion is equivalent to a division by ~66 (65.536),
> so the value of ppb is always smaller than ppm, but not small
> enough to assume narrowing the type from long -> s32 is okay.
> 
> [...]

Here is the summary with links:
  - [net] ptp: improve max_adj check against unreasonable values
    https://git.kernel.org/netdev/net/c/475b92f93216

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html