> -----Original Message-----
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Wednesday, March 28, 2018 10:02
> To: Jon Maloy <jon.ma...@ericsson.com>; Ying Xue
> <ying....@windriver.com>; David S. Miller <da...@davemloft.net>
> Cc: Arnd Bergmann <a...@arndb.de>; Parthasarathy Bhuvaragan
> <parthasarathy.bhuvara...@ericsson.com>; netdev@vger.kernel.org; tipc-
> discuss...@lists.sourceforge.net; linux-ker...@vger.kernel.org
> 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 <a...@arndb.de>
> ---
> 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
