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:     David Miller <davem@davemloft.net>
CC:     "jon.maloy@ericsson.com" <jon.maloy@ericsson.com>,
        "ying.xue@windriver.com" <ying.xue@windriver.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 copying bytes beyond the supplied data
Thread-Topic: [PATCH] tipc: Avoid copying bytes beyond the supplied data
Thread-Index: AQHVAJSPla9XQI+X40m6eIT6p/LBgg==
Date:   Sun, 5 May 2019 20:20:14 +0000
Message-ID: <306471ba2dc54014a77b090d2cf6a7c7@svr-chch-ex1.atlnz.lc>
References: <20190502031004.7125-1-chris.packham@alliedtelesis.co.nz>
 <20190504.004449.945185836330139212.davem@davemloft.net>
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

On 4/05/19 4:45 PM, David Miller wrote:=0A=
> From: Chris Packham <chris.packham@alliedtelesis.co.nz>=0A=
> Date: Thu,  2 May 2019 15:10:04 +1200=0A=
> =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=
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>=0A=
> =0A=
> But now the pad bytes at the end are uninitialized.=0A=
> =0A=
> The whole idea is that the encapsulating TLV object has to be rounded=0A=
> up in size based upon the given 'len' for the data.=0A=
> =0A=
=0A=
TLV_LENGTH() does not account for any padding bytes due to the =0A=
alignment. TLV_SPACE() does but that wasn't used in the code before my =0A=
change.=0A=
=0A=
Are you suggesting something like this=0A=
=0A=
=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) - =0A=
TLV_LENGTH(len));=0A=
+        }=0A=
=0A=
=0A=