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:   Chris Packham <Chris.Packham@alliedtelesis.co.nz>
To:     Ying Xue <ying.xue@windriver.com>,
        "jon.maloy@ericsson.com" <jon.maloy@ericsson.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "niveditas98@gmail.com" <niveditas98@gmail.com>
CC:     "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 v2] tipc: Avoid copying bytes beyond the supplied data
Thread-Topic: [PATCH v2] tipc: Avoid copying bytes beyond the supplied data
Thread-Index: AQHVDr6UzD7ShqRrH0SCh8JWI9STBw==
Date:   Wed, 22 May 2019 20:46:13 +0000
Message-ID: <00ce1b1e52ac4b729d982c86127334aa@svr-chch-ex1.atlnz.lc>
References: <20190520034536.22782-1-chris.packham@alliedtelesis.co.nz>
 <2830aab3-3fa9-36d2-5646-d5e4672ae263@windriver.com>
Accept-Language: en-NZ, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Ying,=0A=
=0A=
On 22/05/19 7:58 PM, Ying Xue wrote:=0A=
> On 5/20/19 11:45 AM, Chris Packham wrote:=0A=
>> TLV_SET is called with a data pointer and a len parameter that tells us=
=0A=
>> how many bytes are pointed to by data. When invoking memcpy() we need=0A=
>> to careful to only copy len bytes.=0A=
>>=0A=
>> Previously we would copy TLV_LENGTH(len) bytes which would copy an extra=
=0A=
>> 4 bytes past the end of the data pointer which newer GCC versions=0A=
>> complain about.=0A=
>>=0A=
>>   In file included from test.c:17:=0A=
>>   In function 'TLV_SET',=0A=
>>       inlined from 'test' at test.c:186:5:=0A=
>>   /usr/include/linux/tipc_config.h:317:3:=0A=
>>   warning: 'memcpy' forming offset [33, 36] is out of the bounds [0, 32]=
=0A=
>>   of object 'bearer_name' with type 'char[32]' [-Warray-bounds]=0A=
>>       memcpy(TLV_DATA(tlv_ptr), data, tlv_len);=0A=
>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~=0A=
>>   test.c: In function 'test':=0A=
>>   test.c::161:10: note:=0A=
>>   'bearer_name' declared here=0A=
>>       char bearer_name[TIPC_MAX_BEARER_NAME];=0A=
>>            ^~~~~~~~~~~=0A=
>>=0A=
>> We still want to ensure any padding bytes at the end are initialised, do=
=0A=
>> this with a explicit memset() rather than copy bytes past the end of=0A=
>> data. Apply the same logic to TCM_SET.=0A=
>>=0A=
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>=0A=
> =0A=
> Acked-by: Ying Xue <ying.xue@windriver.com>=0A=
> =0A=
> =0A=
> But please make the same changes in usr/include/linux/tipc_config.h=0A=
> =0A=
=0A=
I don't understand this comment. There is no usr/include in the kernel =0A=
source.=0A=
=0A=
On most distros that is generated from include/uapi in the kernel source =
=0A=
and packaged as part of libc or a kernel-headers package. So once this =0A=
patch is accepted and makes it into the distros =0A=
/usr/include/linux/tipc_config.h will have this fix.=0A=
=0A=
Adding Cc: stable might help get it there sooner. But I wanted to get it =
=0A=
reviewed/accepted first.=0A=
=0A=
>> ---=0A=
>>=0A=
>> Changes in v2:=0A=
>> - Ensure padding bytes are initialised in both TLV_SET and TCM_SET=0A=
>>=0A=
>>   include/uapi/linux/tipc_config.h | 10 +++++++---=0A=
>>   1 file changed, 7 insertions(+), 3 deletions(-)=0A=
>>=0A=
>> diff --git a/include/uapi/linux/tipc_config.h b/include/uapi/linux/tipc_=
config.h=0A=
>> index 4b2c93b1934c..4955e1a9f1bc 100644=0A=
>> --- a/include/uapi/linux/tipc_config.h=0A=
>> +++ b/include/uapi/linux/tipc_config.h=0A=
>> @@ -307,8 +307,10 @@ static inline int TLV_SET(void *tlv, __u16 type, vo=
id *data, __u16 len)=0A=
>>   	tlv_ptr =3D (struct tlv_desc *)tlv;=0A=
>>   	tlv_ptr->tlv_type =3D htons(type);=0A=
>>   	tlv_ptr->tlv_len  =3D htons(tlv_len);=0A=
>> -	if (len && data)=0A=
>> -		memcpy(TLV_DATA(tlv_ptr), data, tlv_len);=0A=
>> +	if (len && data) {=0A=
>> +		memcpy(TLV_DATA(tlv_ptr), data, len);=0A=
>> +		memset(TLV_DATA(tlv_ptr) + len, 0, TLV_SPACE(len) - tlv_len);=0A=
>> +	}=0A=
>>   	return TLV_SPACE(len);=0A=
>>   }=0A=
>>   =0A=
>> @@ -405,8 +407,10 @@ static inline int TCM_SET(void *msg, __u16 cmd, __u=
16 flags,=0A=
>>   	tcm_hdr->tcm_len   =3D htonl(msg_len);=0A=
>>   	tcm_hdr->tcm_type  =3D htons(cmd);=0A=
>>   	tcm_hdr->tcm_flags =3D htons(flags);=0A=
>> -	if (data_len && data)=0A=
>> +	if (data_len && data) {=0A=
>>   		memcpy(TCM_DATA(msg), data, data_len);=0A=
>> +		memset(TCM_DATA(msg) + data_len, 0, TCM_SPACE(data_len) - msg_len);=
=0A=
>> +	}=0A=
>>   	return TCM_SPACE(data_len);=0A=
>>   }=0A=
>>   =0A=
>>=0A=
> =0A=
=0A=