RE: [PATCH] tipc: avoid possible string overflow

[Jon Maloy]

> -----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