[PATCH] tipc: avoid possible string overflow

[PATCH] tipc: avoid possible string overflow

[Arnd Bergmann]

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.

Fixes: 25b0b9c4e835 ("tipc: handle collisions of 32-bit node address hash values")
Signed-off-by: Arnd Bergmann <arnd@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

RE: [PATCH] tipc: avoid possible string overflow

[Jon Maloy]

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

Re: [PATCH] tipc: avoid possible string overflow

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 28 Mar 2018 16:02:04 +0200

> 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.
> 
> Fixes: 25b0b9c4e835 ("tipc: handle collisions of 32-bit node address hash values")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Based upon the discussion here, it looks like Jon will fix this in a different
way by increasing the destination buffer size.