[PATCH net] net: stmmac: prevent division by 0 in stmmac_init_tstamp_counter()

[PATCH net] net: stmmac: prevent division by 0 in stmmac_init_tstamp_counter()

[Sergey Shtylyov]

In stmmac_init_tstamp_counter(), the sec_inc variable is initialized to 0,
and if stmmac_config_sub_second_increment() fails to set it to some non-0
value, the following div_u64() call would cause a kernel oops (because of
the divide error exception).  Let's check sec_inc for 0 before dividing by
it and just return -EINVAL if so...

Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.

Fixes: df103170854e ("net: stmmac: Avoid sometimes uninitialized Clang warnings")
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
The patch is against the master branch of Linus Torvalds' linux.git repo.

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
===================================================================
--- linux.orig/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ linux/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -817,6 +817,8 @@ int stmmac_init_tstamp_counter(struct st
 	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
 					   priv->plat->clk_ptp_rate,
 					   xmac, &sec_inc);
+	if (!sec_inc)
+		return -EINVAL;
 	temp = div_u64(1000000000ULL, sec_inc);
 
 	/* Store sub second increment for later use */

Re: [PATCH net] net: stmmac: prevent division by 0 in stmmac_init_tstamp_counter()

[Vadim Fedorenko]

On 05/09/2025 17:06, Sergey Shtylyov wrote:
> In stmmac_init_tstamp_counter(), the sec_inc variable is initialized to 0,
> and if stmmac_config_sub_second_increment() fails to set it to some non-0

How that can happen? Do you have real kernel oops log?

Re: [PATCH net] net: stmmac: prevent division by 0 in stmmac_init_tstamp_counter()

[Sergey Shtylyov]

Hello!

On 9/7/25 8:41 PM, Vadim Fedorenko wrote:
[...]
>> In stmmac_init_tstamp_counter(), the sec_inc variable is initialized to 0,
>> and if stmmac_config_sub_second_increment() fails to set it to some non-0
> 
> How that can happen?

   Let's see what the commit in my Fixes tag said about the problem it fixed:

---
When building with -Wsometimes-uninitialized, Clang warns:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:495:3: warning: variable 'ns' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:495:3: warning: variable 'ns' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:532:3: warning: variable 'ns' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:532:3: warning: variable 'ns' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:741:3: warning: variable 'sec_inc' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:741:3: warning: variable 'sec_inc' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]

Clang is concerned with the use of stmmac_do_void_callback (which
stmmac_get_timestamp and stmmac_config_sub_second_increment wrap),
as it may fail to initialize these values if the if condition was ever
false (meaning the callbacks don't exist). It's not wrong because the
callbacks (get_timestamp and config_sub_second_increment respectively)
are the ones that initialize the variables. While it's unlikely that the
callbacks are ever going to disappear and make that condition false, we
can easily avoid this warning by zero initialize the variables.
---

   I think the original commit was just somewhat incomplete, as (adding 0-
initializer into picture) it missed to add checking of sec_inc for 0 before
invoking do_div()...

> Do you have real kernel oops log?

   No, this was just flagged by Svace (the static analyzer):

https://www.ispras.ru/en/technologies/svace/

MBR, Sergey

Re: [PATCH net] net: stmmac: prevent division by 0 in stmmac_init_tstamp_counter()

[Sergey Shtylyov]

On 9/8/25 9:26 PM, Sergey Shtylyov wrote:
[...]

>>> In stmmac_init_tstamp_counter(), the sec_inc variable is initialized to 0,
>>> and if stmmac_config_sub_second_increment() fails to set it to some non-0
>>
>> How that can happen?
> 
>    Let's see what the commit in my Fixes tag said about the problem it fixed:
> 
> ---
> When building with -Wsometimes-uninitialized, Clang warns:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:495:3: warning: variable 'ns' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:495:3: warning: variable 'ns' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:532:3: warning: variable 'ns' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:532:3: warning: variable 'ns' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:741:3: warning: variable 'sec_inc' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:741:3: warning: variable 'sec_inc' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
> 
> Clang is concerned with the use of stmmac_do_void_callback (which
> stmmac_get_timestamp and stmmac_config_sub_second_increment wrap),
> as it may fail to initialize these values if the if condition was ever
> false (meaning the callbacks don't exist). It's not wrong because the
> callbacks (get_timestamp and config_sub_second_increment respectively)
> are the ones that initialize the variables. While it's unlikely that the
> callbacks are ever going to disappear and make that condition false, we
> can easily avoid this warning by zero initialize the variables.
> ---
> 
>    I think the original commit was just somewhat incomplete, as (adding 0-
> initializer into picture) it missed to add checking of sec_inc for 0 before
> invoking do_div()...

   Oops, it was div_u64()! :-)

MBR, Sergey

Re: [PATCH net] net: stmmac: prevent division by 0 in stmmac_init_tstamp_counter()

[Andrew Lunn]

On Fri, Sep 05, 2025 at 07:06:50PM +0300, Sergey Shtylyov wrote:
> In stmmac_init_tstamp_counter(), the sec_inc variable is initialized to 0,
> and if stmmac_config_sub_second_increment() fails to set it to some non-0
> value, the following div_u64() call would cause a kernel oops (because of
> the divide error exception).  Let's check sec_inc for 0 before dividing by
> it and just return -EINVAL if so...
> 
> Found by Linux Verification Center (linuxtesting.org) with the Svace static
> analysis tool.
> 
> Fixes: df103170854e ("net: stmmac: Avoid sometimes uninitialized Clang warnings")
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> ---
> The patch is against the master branch of Linus Torvalds' linux.git repo.

Wrong tree. Please see:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

This also needs reviewing by somebody who know the STMMAC
hardware. There is a comment:

	/* For GMAC3.x, 4.x versions, in "fine adjustement mode" set sub-second
	 * increment to twice the number of nanoseconds of a clock cycle.
	 * The calculation of the default_addend value by the caller will set it
	 * to mid-range = 2^31 when the remainder of this division is zero,
	 * which will make the accumulator overflow once every 2 ptp_clock
	 * cycles, adding twice the number of nanoseconds of a clock cycle :
	 * 2000000000ULL / ptp_clock.

So i'm wondering if the subsecond adjustment is sufficient, the
sec_inc might be zero, and rather than returning an error, the
hardware just needs programming differently?

    Andrew

---
pw-bot: cr

Re: [PATCH net] net: stmmac: prevent division by 0 in stmmac_init_tstamp_counter()

[Sergey Shtylyov]

On 9/8/25 7:47 PM, Andrew Lunn wrote:

>> In stmmac_init_tstamp_counter(), the sec_inc variable is initialized to 0,
>> and if stmmac_config_sub_second_increment() fails to set it to some non-0
>> value, the following div_u64() call would cause a kernel oops (because of
>> the divide error exception).  Let's check sec_inc for 0 before dividing by
>> it and just return -EINVAL if so...
>>
>> Found by Linux Verification Center (linuxtesting.org) with the Svace static
>> analysis tool.
>>
>> Fixes: df103170854e ("net: stmmac: Avoid sometimes uninitialized Clang warnings")
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>
>> ---
>> The patch is against the master branch of Linus Torvalds' linux.git repo.
> 
> Wrong tree. Please see:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

   Well, formerly being a reviewer for the Renesas drivers (before Greg KH
threw me out of MAINTAINERS last year), I kinda know this! :-)
   The real problem is that I've lost the ability to pull from git.kernel.org
(first using git:// protocol and later using https:// as well)... Sometimes
the ability used to return (along with Facebook/LinkedIn -- which are actually
blocked in .ru) but that hasn't happened for more than a week now. I can now
pull from Linus' tree at github. I strongly suspect some interaction between
the .ru blocking and the Anubis program that is now used on git.kernel.org --
I've also lost access to lore.kernel.org (sometimes it works -- but not now)
and also elixir.bootlin.com, both of which seem to use the darn Anubis as
well... :-(

> This also needs reviewing by somebody who know the STMMAC
> hardware. There is a comment:
> 
> 	/* For GMAC3.x, 4.x versions, in "fine adjustement mode" set sub-second
                                               ^ Look, a typo! :-)

> 	 * increment to twice the number of nanoseconds of a clock cycle.
> 	 * The calculation of the default_addend value by the caller will set it
> 	 * to mid-range = 2^31 when the remainder of this division is zero,
> 	 * which will make the accumulator overflow once every 2 ptp_clock
> 	 * cycles, adding twice the number of nanoseconds of a clock cycle :
> 	 * 2000000000ULL / ptp_clock.
> 
> So i'm wondering if the subsecond adjustment is sufficient, the
> sec_inc might be zero, and rather than returning an error, the
> hardware just needs programming differently?

   Sorry, I don't readily see how the data var in config_sub_second_increment()
can wrap to 0. I agree that a look of a more knowledgeable person would be good
though... :-)

>     Andrew

[...]

MBR, Sergey