Subject: [PATCH] changed timespec64_to_ns to avoid underrun

Subject: [PATCH] changed timespec64_to_ns to avoid underrun

[OPENSOURCE Lukas Hannen]

This patch fixes a small oversight in timespec64_to_ns() that has
resulted in negative seconds being erroneously clamped to KTIME_MAX
due to a cast to unsigned long long (which expands to the 2's complement
of a negative long long, even if the architecture does not implement
negative numbers using 2's complement)

This is especially relevant in the PTP context, since the ptp_clock_info
struct (from include/linux/ptp_clock_kernel.h) specifies

        int (*adjtime)(struct ptp_clock_info *ptp, s64 delta);
        int (*gettime64)(struct ptp_clock_info *ptp, struct timespec64 *ts);

which is exactly the kind of timespec64 / nanoseconds mix in combination
with negative values ( ns adjust times ) that can easily lead to calling
timespec64_to_ns with a negative ts->tv_sec, which would in turn lead to
instability of the ptp clock.

Fixes: cb47755725da ("time: Prevent undefined behaviour in timespec64_to_ns()")'
Signed-off-by: Lukas Hannen <lukas.hannen@opensource.tttech-industrial.com>

---
The Patch should apply cleanly to all the branches that the original
commit cb47755725da ("time: Prevent undefined behaviour in timespec64_to_ns()")'
was backported to.

include/linux/time64.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/time64.h b/include/linux/time64.h
index 5117cb5b56561..81b9686a20799 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -21,15 +21,17 @@ struct itimerspec64 {
 };

 /* Located here for timespec[64]_valid_strict */
 #define TIME64_MAX                     ((s64)~((u64)1 << 63))
 #define TIME64_MIN                     (-TIME64_MAX - 1)

 #define KTIME_MAX                      ((s64)~((u64)1 << 63))
+#define KTIME_MIN                      (-KTIME_MAX - 1)
 #define KTIME_SEC_MAX                  (KTIME_MAX / NSEC_PER_SEC)
+#define KTIME_SEC_MIN                  (KTIME_MIN / NSEC_PER_SEC)

 /*
  * Limits for settimeofday():
  *
  * To prevent setting the time close to the wraparound point time setting
  * is limited so a reasonable uptime can be accomodated. Uptime of 30 years
  * should be really sufficient, which means the cutoff is 2232. At that
@@ -120,18 +122,21 @@ static inline bool timespec64_valid_settod(const struct timespec64 *ts)
  * @ts:                pointer to the timespec64 variable to be converted
  *
  * Returns the scalar nanosecond representation of the timespec64
  * parameter.
  */
 static inline s64 timespec64_to_ns(const struct timespec64 *ts)
 {
-       /* Prevent multiplication overflow */
-       if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX)
+       /* Prevent multiplication overflow / underflow */
+       if (ts->tv_sec >= KTIME_SEC_MAX)
                return KTIME_MAX;

+       if (ts->tv_sec <= KTIME_SEC_MIN)
+               return KTIME_MIN;
+
        return ((s64) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
 }

 /**
  * ns_to_timespec64 - Convert nanoseconds to timespec64
  * @nsec:      the nanoseconds value to be converted
  *
--
2.31.1


Internal

Re: Subject: [PATCH] changed timespec64_to_ns to avoid underrun

[Thomas Gleixner]

Lukas,

On Wed, Aug 25 2021 at 10:12, OPENSOURCE Lukas Hannen wrote:

thanks for the patch. A few formal nitpicks:

The subject line lacks a subsystem prefix. You can figure that 
usually out by running:

   git log --format=oneline --abbrev-commit include/linux/time64.h

Look for the most used. So in this case it would be simply 'time:'

'changed ... underrun'

Please use imperative mood, i.e. 'Change'. But 'change' is redundant
here anyway because it would not be a patch if it would not change
anything.

So something like: 'Handle negative seconds correctly in timespec64_to_ns()'
would be more informative. Your subject line does not really match
the problem.

Also note the brackets which make it obvious that this talks about a
function.

You have a redundant 'Subject:' in the Subject: header

> This patch fixes a small oversight in timespec64_to_ns() that has

This patch is redundant as well. See Documentation/process/ and grep for
'This patch'

> resulted in negative seconds being erroneously clamped to KTIME_MAX
> due to a cast to unsigned long long (which expands to the 2's complement
> of a negative long long, even if the architecture does not implement
> negative numbers using 2's complement)
>
> This is especially relevant in the PTP context, since the ptp_clock_info
> struct (from include/linux/ptp_clock_kernel.h) specifies
>
>	int (*adjtime)(struct ptp_clock_info *ptp, s64 delta);
>	int (*gettime64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
> 
> which is exactly the kind of timespec64 / nanoseconds mix in combination
> with negative values ( ns adjust times ) that can easily lead to calling
> timespec64_to_ns with a negative ts->tv_sec, which would in turn lead to
> instability of the ptp clock.

This is confusing at best.

The adjtime() callback has nothing to do with timespec64_to_ns(). The
conversion happens in the calling code.

gettime64() neither because that reads the time from the PTP clock which
cannot be negative and nothing uses timespec64_to_ns() there either.

The place where a negative seconds value must be handled correctly is
ptp_clock_adjtime().


So something like this:

timespec64_ns() prevents multiplication overflows by comparing the
seconds value of the timespec to KTIME_SEC_MAX. If the value is greater
or equal it returns KTIME_MAX.

But that check casts the signed seconds value to unsigned which makes
the comparision true for all negative values and therefore return
wrongly KTIME_MAX.

Negative second values are perfectly valid and required in some places,
e.g. ptp_clock_adjtime().

Remove the cast and add a check for the negative boundary which is
required to prevent undefined behaviour of the multiplication due to
multiplication underflow.

Hmm?

> Fixes: cb47755725da ("time: Prevent undefined behaviour in timespec64_to_ns()")'
> Signed-off-by: Lukas Hannen <lukas.hannen@opensource.tttech-industrial.com>
>
> ---
> The Patch should apply cleanly to all the branches that the original

Emphasis on should. See below.

> commit cb47755725da ("time: Prevent undefined behaviour in timespec64_to_ns()")'
> was backported to.
>
> include/linux/time64.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/time64.h b/include/linux/time64.h
> index 5117cb5b56561..81b9686a20799 100644
> --- a/include/linux/time64.h
> +++ b/include/linux/time64.h
> @@ -21,15 +21,17 @@ struct itimerspec64 {
>  };
>
>  /* Located here for timespec[64]_valid_strict */
>  #define TIME64_MAX                     ((s64)~((u64)1 << 63))
>  #define TIME64_MIN                     (-TIME64_MAX - 1)
>
>  #define KTIME_MAX                      ((s64)~((u64)1 << 63))
> +#define KTIME_MIN                      (-KTIME_MAX - 1)
>  #define KTIME_SEC_MAX                  (KTIME_MAX / NSEC_PER_SEC)
> +#define KTIME_SEC_MIN                  (KTIME_MIN / NSEC_PER_SEC)

Your patch is white space damaged, which makes it fail to apply.
Something replaced all tabs with spaces, most likely your mail client.

Please figure out a way to send patches either via git-email or by using
a email client which tries not to be smarter than the person using it.
Send the patch to yourself and validate that it applies cleanly.

I fixed it up for you this time.

Thanks,

        tglx

[tip: timers/urgent] time: Handle negative seconds correctly in timespec64_to_ns()

[tip-bot2 for Lukas Hannen]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     39ff83f2f6cc5cc1458dfcea9697f96338210beb
Gitweb:        https://git.kernel.org/tip/39ff83f2f6cc5cc1458dfcea9697f96338210beb
Author:        Lukas Hannen <lukas.hannen@opensource.tttech-industrial.com>
AuthorDate:    Wed, 25 Aug 2021 10:12:43 
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 08 Sep 2021 17:44:26 +02:00

time: Handle negative seconds correctly in timespec64_to_ns()

timespec64_ns() prevents multiplication overflows by comparing the seconds
value of the timespec to KTIME_SEC_MAX. If the value is greater or equal it
returns KTIME_MAX.

But that check casts the signed seconds value to unsigned which makes the
comparision true for all negative values and therefore return wrongly
KTIME_MAX.

Negative second values are perfectly valid and required in some places,
e.g. ptp_clock_adjtime().

Remove the cast and add a check for the negative boundary which is required
to prevent undefined behaviour due to multiplication underflow.

Fixes: cb47755725da ("time: Prevent undefined behaviour in timespec64_to_ns()")'
Signed-off-by: Lukas Hannen <lukas.hannen@opensource.tttech-industrial.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/AM6PR01MB541637BD6F336B8FFB72AF80EEC69@AM6PR01MB5416.eurprd01.prod.exchangelabs.com
---
 include/linux/time64.h |  9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/time64.h b/include/linux/time64.h
index 5117cb5..81b9686 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -25,7 +25,9 @@ struct itimerspec64 {
 #define TIME64_MIN			(-TIME64_MAX - 1)
 
 #define KTIME_MAX			((s64)~((u64)1 << 63))
+#define KTIME_MIN			(-KTIME_MAX - 1)
 #define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
+#define KTIME_SEC_MIN			(KTIME_MIN / NSEC_PER_SEC)
 
 /*
  * Limits for settimeofday():
@@ -124,10 +126,13 @@ static inline bool timespec64_valid_settod(const struct timespec64 *ts)
  */
 static inline s64 timespec64_to_ns(const struct timespec64 *ts)
 {
-	/* Prevent multiplication overflow */
-	if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX)
+	/* Prevent multiplication overflow / underflow */
+	if (ts->tv_sec >= KTIME_SEC_MAX)
 		return KTIME_MAX;
 
+	if (ts->tv_sec <= KTIME_SEC_MIN)
+		return KTIME_MIN;
+
 	return ((s64) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
 }
 

RE: [tip: timers/urgent] time: Handle negative seconds correctly in timespec64_to_ns()

[David Laight]

> Committer:     Thomas Gleixner <tglx@linutronix.de>
> CommitterDate: Wed, 08 Sep 2021 17:44:26 +02:00
> 
> time: Handle negative seconds correctly in timespec64_to_ns()
> 
> timespec64_ns() prevents multiplication overflows by comparing the seconds
> value of the timespec to KTIME_SEC_MAX. If the value is greater or equal it
> returns KTIME_MAX.
> 
> But that check casts the signed seconds value to unsigned which makes the
> comparision true for all negative values and therefore return wrongly
> KTIME_MAX.
> 
> Negative second values are perfectly valid and required in some places,
> e.g. ptp_clock_adjtime().
> 
> Remove the cast and add a check for the negative boundary which is required
> to prevent undefined behaviour due to multiplication underflow.
> 
> Fixes: cb47755725da ("time: Prevent undefined behaviour in timespec64_to_ns()")'
> Signed-off-by: Lukas Hannen <lukas.hannen@opensource.tttech-industrial.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> Link:
> https://lore.kernel.org/r/AM6PR01MB541637BD6F336B8FFB72AF80EEC69@AM6PR01MB5416.eurprd01.prod.exchangel
> abs.com
> ---
>  include/linux/time64.h |  9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/time64.h b/include/linux/time64.h
> index 5117cb5..81b9686 100644
> --- a/include/linux/time64.h
> +++ b/include/linux/time64.h
> @@ -25,7 +25,9 @@ struct itimerspec64 {
>  #define TIME64_MIN			(-TIME64_MAX - 1)
> 
>  #define KTIME_MAX			((s64)~((u64)1 << 63))
> +#define KTIME_MIN			(-KTIME_MAX - 1)
>  #define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
> +#define KTIME_SEC_MIN			(KTIME_MIN / NSEC_PER_SEC)
> 
>  /*
>   * Limits for settimeofday():
> @@ -124,10 +126,13 @@ static inline bool timespec64_valid_settod(const struct timespec64 *ts)
>   */
>  static inline s64 timespec64_to_ns(const struct timespec64 *ts)
>  {
> -	/* Prevent multiplication overflow */
> -	if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX)
> +	/* Prevent multiplication overflow / underflow */
> +	if (ts->tv_sec >= KTIME_SEC_MAX)
>  		return KTIME_MAX;
> 
> +	if (ts->tv_sec <= KTIME_SEC_MIN)
> +		return KTIME_MIN;
> +
>  	return ((s64) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
>  }

Adding tv_nsec can still overflow -  even if tv_nsec is bounded to +/- 1 second.
This is no more 'garbage in' => 'garbage out' than the code without the
multiply under/overflow check.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [tip: timers/urgent] time: Handle negative seconds correctly in timespec64_to_ns()

[Thomas Gleixner]

David,

On Wed, Sep 08 2021 at 16:01, David Laight wrote:
>> +	if (ts->tv_sec <= KTIME_SEC_MIN)
>> +		return KTIME_MIN;
>> +
>>  	return ((s64) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
>>  }
>
> Adding tv_nsec can still overflow -  even if tv_nsec is bounded to +/- 1 second.
> This is no more 'garbage in' => 'garbage out' than the code without the
> multiply under/overflow check.

In kernel timespecs are always normalized:  0 < tv_nsec < 1e9 - 1

Let's do the math:

  KTIME_SEC_MAX = KTIME_MAX / NSEC_PER_SEC

  The overflow prevention does:

    if PSVAL >= KTIME_SEC_MAX:
       return KTIME_MAX

  so the largest positive seconds value which passes the above is:

    PSMAX = KTIME_SEC_MAX - 1

  ergo:

    PSMAX * NSEC_PER_SEC + (NSEC_PER_SEC - 1) < KTIME_SEC_MAX < KTIME_MAX

I leave the proof for negative values as an excercise for the reader.

Thanks,

        tglx
---
"Math is hard, let's go shopping!" - John Stultz