RE: [PATCH] tipc: avoid possible string overflow

From: Jon Maloy
Date: Wed Mar 28 2018 - 10:42:22 EST




> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> Sent: Wednesday, March 28, 2018 10:02
> To: Jon Maloy <jon.maloy@xxxxxxxxxxxx>; Ying Xue
> <ying.xue@xxxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>; Parthasarathy Bhuvaragan
> <parthasarathy.bhuvaragan@xxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; tipc-
> discussion@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [PATCH] tipc: avoid possible string overflow
>
> gcc points out that the combined length of the fixed-length inputs to
> l->name is larger than the destination buffer size:
>
> net/tipc/link.c: In function 'tipc_link_create':
> net/tipc/link.c:465:26: error: '%s' directive writing up to 32 bytes into a region
> of size between 26 and 58 [-Werror=format-overflow=]
> sprintf(l->name, "%s:%s-%s:unknown", self_str, if_name, peer_str);
> ^~ ~~~~~~~~
> net/tipc/link.c:465:2: note: 'sprintf' output 11 or more bytes (assuming 75)
> into a destination of size 60
> sprintf(l->name, "%s:%s-%s:unknown", self_str, if_name, peer_str);
>
> Using snprintf() ensures that the destination is still a nul-terminated string in
> all cases. It's still theoretically possible that the string gets trunctated though,
> so this patch should be carefully reviewed to ensure that either truncation is
> impossible in practice, or that we're ok with the truncation.

Theoretically, maximum bearer name is MAX_BEARER_NAME - 3 = 29 (because if_name is only the part after the ":" in a bearer name, and is zero-terminated.
The lines just above in the code reveals that the maximum length of self_str and peer_str is 16.
This taken together means that the theoretically max length of a link name becomes:
16 + 1 + 29 + 1 + 16 + 1 + 29 = 93. Since we also need room for a terminating zero, we need to extend the tipc_link::name array to 96 bytes.

I'll fix that.

Thank you to for reporting this.
///jon

>
> Fixes: 25b0b9c4e835 ("tipc: handle collisions of 32-bit node address hash
> values")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> net/tipc/link.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c index 1289b4ba404f..c195ba036035
> 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -462,7 +462,8 @@ bool tipc_link_create(struct net *net, char *if_name,
> int bearer_id,
> sprintf(peer_str, "%x", peer);
> }
> /* Peer i/f name will be completed by reset/activate message */
> - sprintf(l->name, "%s:%s-%s:unknown", self_str, if_name, peer_str);
> + snprintf(l->name, sizeof(l->name), "%s:%s-%s:unknown",
> + self_str, if_name, peer_str);
>
> strcpy(l->if_name, if_name);
> l->addr = peer;
> --
> 2.9.0