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

[Tung Quang Nguyen]

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

This patch targets net tree, please add 'net' to the patch title: [PATCH net]

>
>Fix two validation flaws in `net/tipc/netlink_compat.c`:

Two different bugs require 2 separates patches. Please pay attention to this next time.

>
>1. `tipc_nl_compat_net_set()` (`TIPC_CMD_SET_NETID` /
>   `TIPC_CMD_SET_NODE_ADDR`) unconditionally dereferences `*(__be32
>   *)TLV_DATA(msg->req)` without verifying that
>   `TLV_GET_DATA_LEN(msg->req) >= sizeof(__be32)` (4 bytes), reading 4
>   bytes past the end of a 0-byte TLV payload (`tlv_len == 4`).
>2. In `tipc_nl_compat_name_table_dump_header()` and
>   `tipc_nl_compat_name_table_dump()`, `ntq->depth` can contain the high
>   flag bit `TIPC_NTQ_ALLTYPES` (`0x80000000`).
>   `tipc_nl_compat_name_table_dump_header()` compares `depth > 4`
>   without masking off `TIPC_NTQ_ALLTYPES` first (always clamping
>   `depth` to 4 whenever `TIPC_NTQ_ALLTYPES` is set, even if the low
>   depth bits are `1`), and `tipc_nl_compat_name_table_dump()` checks
>   `depth > 1` and `depth > 2` with `TIPC_NTQ_ALLTYPES` still set in
>   `depth`. Mask off `TIPC_NTQ_ALLTYPES` after testing it.
>
>Fixes: d7cc75d3cb6b ("tipc: convert legacy nl node addr set to nl compat")
>Fixes: 44a8ae94fd55 ("tipc: convert legacy nl name table dump to nl compat")
>Assisted-by: LLM
>Signed-off-by: Hui Peng <benquike@xxxxxxxxx>
>
>---
> net/tipc/netlink_compat.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>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;
>@@ -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;

After you mask off ' depth', the subsequent code does a bitwise operation using TIPC_NTQ_ALLTYPES:
" if (!(depth & TIPC_NTQ_ALLTYPES) &&
            (type != nla_get_u32(publ[TIPC_NLA_PUBL_TYPE])))
"
Does your change break above code ?

> 	if (lowbound && (lowbound >
>nla_get_u32(publ[TIPC_NLA_PUBL_UPPER])))
> 		return 0;
> 	if (upbound && (upbound <
>nla_get_u32(publ[TIPC_NLA_PUBL_LOWER])))
>@@ -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));
>
> 	net = nla_nest_start_noflag(skb, TIPC_NLA_NET);
>--
>2.55.0.1082.g2b9226bbc0-goog