Re: [PATCH] tipc: fix TLV out-of-bounds read in tipc_nl_compat_net_set() and depth mask

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 1

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 mischaracterizes the code it changes.
- [Medium] The new mask introduces a header/row mismatch for a request
  whose depth field is exactly TIPC_NTQ_ALLTYPES (0x80000000, flag set…
- [Low] Two unrelated fixes with two different Fixes: tags are bundled in
  one commit.

Pre-existing issues:
- [Medium] When TIPC_NTQ_ALLTYPES is set,
  tipc_nl_compat_name_table_dump() only bypasses the type comparison…

--

tipc: fix TLV out-of-bounds read in tipc_nl_compat_net_set() and depth mask

This adds a missing TLV payload length check in tipc_nl_compat_net_set()
before dereferencing the request payload as a __be32, and masks
TIPC_NTQ_ALLTYPES out of the display depth in
tipc_nl_compat_name_table_dump_header() and
tipc_nl_compat_name_table_dump().

A few questions below about the commit message and about the masking hunks.

[Severity: Medium]
The commit message says:

    `tipc_nl_compat_name_table_dump()` checks `depth > 1` and `depth > 2`
    with `TIPC_NTQ_ALLTYPES` still set in `depth`

Is that description accurate?  tipc_nl_compat_name_table_dump() uses
equality tests, not greater-than tests:

	if (depth == 1)
		goto out;
	...
	if (depth == 2)
		goto out;
	...
	if (depth == 3)
		goto out;

With TIPC_NTQ_ALLTYPES set, none of those equality tests matched before
this patch, so all four columns were printed, which agreed with the
header path where 0x80000000 > 4 clamped the title count to four.

Given that, isn't the real effect of the masking hunks to start honouring
the low depth bits for ALLTYPES requests, that is, to print fewer columns
than before (depth = TIPC_NTQ_ALLTYPES | 1 now yields one column instead
of four)?  Could the changelog describe that behaviour change instead?

The message also calls both changes "validation flaws".  In
tipc_nl_compat_name_table_dump_header() the header[] array is only indexed
after the clamp:

	if (depth > 4)
		depth = 4;
	for (i = 0; i < depth; i++)
		tipc_tlv_sprintf(msg->rep, header[i]);

so there is no out-of-bounds access in that path.  Should the depth-mask
part be described as an output formatting change rather than a validation
or memory-safety fix?

> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index d9a4f94ea2d4..9b2d93704e23 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -881,7 +881,7 @@ static int tipc_nl_compat_name_table_dump_header(struct tipc_nl_compat_msg *msg)
>  	if (TLV_GET_DATA_LEN(msg->req) < (int)sizeof(struct tipc_name_table_query))
>  		return -EINVAL;
>  
> -	depth = ntohl(ntq->depth);
> +	depth = ntohl(ntq->depth) & ~TIPC_NTQ_ALLTYPES;
>  
>  	if (depth > 4)
>  		depth = 4;

[Severity: Medium]
What happens for a request whose depth field is exactly TIPC_NTQ_ALLTYPES,
that is the flag bit set and all low depth bits zero?

In tipc_nl_compat_name_table_dump_header() the masked depth becomes 0, so
the clamp does not trigger, the title loop body never runs, and only "\n"
is emitted:

	depth = ntohl(ntq->depth) & ~TIPC_NTQ_ALLTYPES;	/* 0 */

	if (depth > 4)					/* false */
		depth = 4;
	for (i = 0; i < depth; i++)			/* no iterations */
		tipc_tlv_sprintf(msg->rep, header[i]);
	tipc_tlv_sprintf(msg->rep, "\n");

Meanwhile the row formatter below stops only on depth == 1, 2 or 3, none
of which match 0, so every publication still prints all four columns.
Does that leave the report with an empty header line followed by
four-column rows?

Before this patch the same request was self-consistent: 0x80000000 > 4
clamped the header to four titles and the rows printed four columns.

The legacy implementation that this code replaced handled depth 0
explicitly, in nametbl_list() in net/tipc/name_table.c before commit
44a8ae94fd55:

	depth = (depth_info & ~TIPC_NTQ_ALLTYPES);
	if (depth == 0)
		return 0;

Would it make sense to normalise or reject a masked depth of 0 here, or to
have the row formatter use the same clamp semantics as the header?

> @@ -932,6 +932,7 @@ static int tipc_nl_compat_name_table_dump(struct tipc_nl_compat_msg *msg,
>  	if (!(depth & TIPC_NTQ_ALLTYPES) &&
>  	    (type != nla_get_u32(publ[TIPC_NLA_PUBL_TYPE])))
>  		return 0;
> +	depth &= ~TIPC_NTQ_ALLTYPES;
>  	if (lowbound && (lowbound > nla_get_u32(publ[TIPC_NLA_PUBL_UPPER])))
>  		return 0;
>  	if (upbound && (upbound < nla_get_u32(publ[TIPC_NLA_PUBL_LOWER])))
>  		return 0;

[Severity: Medium]
This is a pre-existing issue and not something introduced by this patch,
but since the new mask lands directly above these two bounds checks it
seems worth raising.

TIPC_NTQ_ALLTYPES only bypasses the type comparison; the lowbound and
upbound filters still run, so publications get dropped from an "all types"
dump.  The uAPI contract in include/uapi/linux/tipc_config.h says all
three selectors are ignored when the high depth bit is set:

	__be32 type;	/* {t,l,u} info ignored if high bit of "depth" is set */
	__be32 lowbound; /* (i.e. displays all entries of name table) */
	__be32 upbound;

With depth = TIPC_NTQ_ALLTYPES | 1 and lowbound = 200, does

	if (lowbound && (lowbound > nla_get_u32(publ[TIPC_NLA_PUBL_UPPER])))
		return 0;

still skip every publication whose upper bound is below 200, even though
lowbound should be irrelevant for that request?

The legacy code enforced this, in nametbl_list() before commit
44a8ae94fd55:

	if (all_types) {
		...
		lowbound = 0;
		upbound = ~0;
		...
	}

Should the lowbound/upbound tests be gated on the ALLTYPES flag as well?

> @@ -1139,6 +1140,9 @@ static int tipc_nl_compat_net_set(struct tipc_nl_compat_cmd_doit *cmd,
>  	u32 val;
>  	struct nlattr *net;
>  
> +	if (TLV_GET_DATA_LEN(msg->req) < (int)sizeof(__be32))
> +		return -EINVAL;
> +
>  	val = ntohl(*(__be32 *)TLV_DATA(msg->req));

[Severity: Low]
Could this be split into two patches?

This hunk is the out-of-bounds read fix in tipc_nl_compat_net_set(), where
only TLV_CHECK_TYPE() ran before it from tipc_nl_compat_doit(), and it
matches Fixes: d7cc75d3cb6b.  The two earlier hunks only change how
TIPC_NTQ_ALLTYPES is masked out of the display depth for
TIPC_CMD_SHOW_NAME_TABLE and match Fixes: 44a8ae94fd55.

The two changes touch disjoint commands: TIPC_CMD_SET_NODE_ADDR and
TIPC_CMD_SET_NETID versus TIPC_CMD_SHOW_NAME_TABLE.  Splitting them would
let the memory-safety fix be backported and reverted independently of the
output format change.

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