Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jon Maloy <jon.maloy@ericsson.com>
To:     Arnd Bergmann <arnd@arndb.de>, Ying Xue <ying.xue@windriver.com>,
        "David S. Miller" <davem@davemloft.net>
CC:     Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "tipc-discussion@lists.sourceforge.net" 
        <tipc-discussion@lists.sourceforge.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] tipc: avoid possible string overflow
Thread-Topic: [PATCH] tipc: avoid possible string overflow
Thread-Index: AQHTxp2NVpY4c8ilWEu176m6nvN3U6Pls/pw
Date:   Wed, 28 Mar 2018 14:41:50 +0000
Message-ID: <BN6PR15MB1553BD0EDB723303EA5FAC6E9AA30@BN6PR15MB1553.namprd15.prod.outlook.com>
References: <20180328140302.2594031-1-arnd@arndb.de>
In-Reply-To: <20180328140302.2594031-1-arnd@arndb.de>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: ericsson.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 99078834-373b-4d30-46b0-08d594ba0ad0
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Mar 2018 14:41:50.8532
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 92e84ceb-fbfd-47ab-be52-080c6b87953f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR15MB1905
X-OriginatorOrg: ericsson.com
X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTYRjHe8/Z5XgZvS6dD1pJi2A5r0Nrht2oYB9ShKC8RDn1oDbdxqaW
        CaFUECpW3sJlOUuFiWjqLHOBTebKC4mXkIwmoeKt0jQyEiPPzoK+/Z7n/3/e58JLkcKnXD8q
        U51D69TKLDHPnVMT/+JQcIfJkhjWvxEg3yq38+W1I7c48vGeWp7cbhTJzSNX5DOTk+QJnmLz
        dzlSmE0fCIXFuE4o1jv2KtrMXUQcN9E9Oo3OysyjdaHHkt0zXnbM8bUO0bUe+zAqRE3CYuRG
        AY6A5Yd2VIzcKSG2ISibqSbYwIxgsOynS9lAYHyyxGdKhLiRgNWbyYzAwWsEGOZK+KyrgoCN
        6XeuwI6gq8zkLOFhCSxW30EMe2MNjBU3OU0kvkvAwI96khF24SiYrvy8LVDbpiPQ/1rGogw2
        F2IYBwcfgEeOfh7DAnwR7IZRHjtRBHRN1Dmfd8ORMF8x5GyLsAg2BlsIhknsC1OzdQS7NIaG
        VyMkyz6wOPOHy7RCOAGG5hLY9D6wdj/jsrwHxupKnJcAbCagsLPdVRsMq1VVLo4B6/AAlzW9
        RWD7YnYJgdA+YSOZBoBV8KbxKosFUDbseQ+FG/6bjuUgMFrWeCxLoal+mTQ4N/aCgZpZjhFx
        mpGPntanZKfLZCG0LjNVr9eoQ9R0Tgfa/jpW82ZYN1qcP9mHMIXEnoLi+5ZEIVeZp8/P7kNA
        kWJvwYpkOyVIU+Zfp3Way7rcLFrfh/wpjthXII/tTBTidGUOraJpLa37pxKUm18hurAk0B48
        m9Ks0kxHVgo+fi04nl3KUy84OkW/4pYLRt9XGnbmSk2t1UdbZb3npeagpBVS4RN7w6YJ8OrN
        EAU9l15qSw0xLawItXSvpEGyI3kxqXTKY35/KDfDQ9lSVKTKU3me2Tr1beTxp3OOgsNtAdbx
        6N234+cfnC4XRX33l4o5+gxleCCp0yv/ApVpKZo2AwAA
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----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
>=20
> gcc points out that the combined length of the fixed-length inputs to
> l->name is larger than the destination buffer size:
>=20
> 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=3Dformat-overflow=3D]
>   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);
>=20
> Using snprintf() ensures that the destination is still a nul-terminated s=
tring in
> all cases. It's still theoretically possible that the string gets truncta=
ted though,
> so this patch should be carefully reviewed to ensure that either truncati=
on is
> impossible in practice, or that we're ok with the truncation.

Theoretically, maximum bearer name is MAX_BEARER_NAME - 3  =3D 29  (because=
 if_name is only the part after the ":"  in a bearer name, and is zero-term=
inated.
The lines just above in the code reveals that the maximum length of self_st=
r 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 =3D 93.  Since we also need room for a termi=
nating zero, we need to extend the tipc_link::name array to 96 bytes.

I'll fix that.

Thank you to for reporting this.
///jon

>=20
> 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(-)
>=20
> diff --git a/net/tipc/link.c b/net/tipc/link.c index 1289b4ba404f..c195ba=
036035
> 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);
>=20
>  	strcpy(l->if_name, if_name);
>  	l->addr =3D peer;
> --
> 2.9.0