Re: [PATCH net] tipc: fix division by zero in perc()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message's premise is wrong.
- [Medium] Judged by its own premise, the fix is incomplete.

--

tipc: fix division by zero in perc()

This commit adds a zero check to perc() in net/tipc/netlink_compat.c so
that it returns 0 when total is zero. The commit message says this
prevents a division by zero in tipc_nl_compat_link_stat_dump() when link
statistics are queried on a fresh link.

> The perc() function divides count by total without checking whether
> total is zero. When link statistics are queried on a freshly
> established link that has not yet exchanged messages,
> TIPC_NLA_STATS_MSG_PROF_TOT is zero, causing a division by zero
> in tipc_nl_compat_link_stat_dump().

[Severity: Medium]
Can TIPC_NLA_STATS_MSG_PROF_TOT actually be zero here?

This attribute is only produced by __tipc_nl_add_stats() in
net/tipc/link.c, and that function already clamps the value to 1:

net/tipc/link.c:__tipc_nl_add_stats() {
    ...
		{TIPC_NLA_STATS_MSG_PROF_TOT, (s->msg_length_counts) ?
			s->msg_length_counts : 1},
    ...
}

So a link with no profiled messages reports 1, not 0. If msg_length_counts
wraps back to 0, it is also clamped to 1.

User space can't supply these attributes either. __tipc_nl_compat_dumpit()
allocates a kernel skb with nlmsg_new(), fills it through cmd->dumpit, and
passes the parsed attributes to cmd->format:

__tipc_nl_compat_dumpit()
    tipc_nl_node_dump_link()
        __tipc_nl_add_link()
            __tipc_nl_add_stats()
    tipc_nl_compat_link_stat_dump()
        perc()

Broadcast links use __tipc_nl_add_bc_link_stat() and return early through
__fill_bc_link_stat(), before any perc() call.

The clamp came in with commit 7be57fc69184 ("tipc: add link get/dump to
new netlink api"), which is older than the Fixes: target f2b3b2d4ccbf
("tipc: convert legacy nl link stat to nl compat").

If so, the described division by zero has never been reachable. Could the
commit message describe this as hardening instead? Could the Fixes: tag
also be dropped, so stable doesn't pick this up as a crash fix?

> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index d9a4f94ea2d43..75f3193a2a6cf 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -497,6 +497,9 @@ static int tipc_nl_compat_bearer_disable(struct tipc_nl_compat_cmd_doit *cmd,
>  
>  static inline u32 perc(u32 count, u32 total)
>  {
> +	if (!total)
> +		return 0;
> +
>  	return (count * 100 + (total / 2)) / total;
>  }

[Severity: Medium]
If the premise in the commit message were true, would this check ever be
reached?

tipc_nl_compat_link_stat_dump() also divides by the same attribute with no
guard. That division is in a tipc_tlv_sprintf() call that runs before any
perc() call:

net/tipc/netlink_compat.c:tipc_nl_compat_link_stat_dump() {
    ...
	tipc_tlv_sprintf(msg->rep,
			 "  TX profile sample:%u packets  average:%u octets\n",
			 nla_get_u32(stats[TIPC_NLA_STATS_MSG_LEN_CNT]),
			 nla_get_u32(stats[TIPC_NLA_STATS_MSG_LEN_TOT]) /
			 nla_get_u32(stats[TIPC_NLA_STATS_MSG_PROF_TOT]));

	tipc_tlv_sprintf(msg->rep,
			 "  0-64:%u%% -256:%u%% -1024:%u%% -4096:%u%% ",
			 perc(nla_get_u32(stats[TIPC_NLA_STATS_MSG_LEN_P0]),
			      nla_get_u32(stats[TIPC_NLA_STATS_MSG_PROF_TOT])),
    ...
}

If TIPC_NLA_STATS_MSG_PROF_TOT were zero, the divide error would hit in the
average calculation. The new check in perc() would never run.

In practice, the clamp in __tipc_nl_add_stats() keeps both divisions safe.
That makes the patch unnecessary if the clamp is relied on, and incomplete
if it isn't.

Would it be better to drop the crash claim? Alternatively,
TIPC_NLA_STATS_MSG_PROF_TOT could be checked once in
tipc_nl_compat_link_stat_dump(), which would cover both the average and
the perc() calls.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103225433.2663943-1-tristmd%40gmail.com