[PATCH] tipc: Avoid copying bytes beyond the supplied data

[PATCH] tipc: Avoid copying bytes beyond the supplied data

[Chris Packham]

TLV_SET is called with a data pointer and a len parameter that tells us
how many bytes are pointed to by data. When invoking memcpy() we need
to careful to only copy len bytes.

Previously we would copy TLV_LENGTH(len) bytes which would copy an extra
4 bytes past the end of the data pointer which newer GCC versions
complain about.

 In file included from test.c:17:
 In function 'TLV_SET',
     inlined from 'test' at test.c:186:5:
 /usr/include/linux/tipc_config.h:317:3:
 warning: 'memcpy' forming offset [33, 36] is out of the bounds [0, 32]
 of object 'bearer_name' with type 'char[32]' [-Warray-bounds]
     memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 test.c: In function 'test':
 test.c::161:10: note:
 'bearer_name' declared here
     char bearer_name[TIPC_MAX_BEARER_NAME];
          ^~~~~~~~~~~

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 include/uapi/linux/tipc_config.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/tipc_config.h b/include/uapi/linux/tipc_config.h
index 4b2c93b1934c..f65c5b80ed33 100644
--- a/include/uapi/linux/tipc_config.h
+++ b/include/uapi/linux/tipc_config.h
@@ -308,7 +308,7 @@ static inline int TLV_SET(void *tlv, __u16 type, void *data, __u16 len)
 	tlv_ptr->tlv_type = htons(type);
 	tlv_ptr->tlv_len  = htons(tlv_len);
 	if (len && data)
-		memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
+		memcpy(TLV_DATA(tlv_ptr), data, len);
 	return TLV_SPACE(len);
 }
 
-- 
2.21.0

Re: [PATCH] tipc: Avoid copying bytes beyond the supplied data

[David Miller]

From: Chris Packham <chris.packham@alliedtelesis.co.nz>
Date: Thu,  2 May 2019 15:10:04 +1200

> TLV_SET is called with a data pointer and a len parameter that tells us
> how many bytes are pointed to by data. When invoking memcpy() we need
> to careful to only copy len bytes.
> 
> Previously we would copy TLV_LENGTH(len) bytes which would copy an extra
> 4 bytes past the end of the data pointer which newer GCC versions
> complain about.
> 
>  In file included from test.c:17:
>  In function 'TLV_SET',
>      inlined from 'test' at test.c:186:5:
>  /usr/include/linux/tipc_config.h:317:3:
>  warning: 'memcpy' forming offset [33, 36] is out of the bounds [0, 32]
>  of object 'bearer_name' with type 'char[32]' [-Warray-bounds]
>      memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  test.c: In function 'test':
>  test.c::161:10: note:
>  'bearer_name' declared here
>      char bearer_name[TIPC_MAX_BEARER_NAME];
>           ^~~~~~~~~~~
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

But now the pad bytes at the end are uninitialized.

The whole idea is that the encapsulating TLV object has to be rounded
up in size based upon the given 'len' for the data.

Re: [PATCH] tipc: Avoid copying bytes beyond the supplied data

[Chris Packham]

On 4/05/19 4:45 PM, David Miller wrote:
> From: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Date: Thu,  2 May 2019 15:10:04 +1200
> 
>> TLV_SET is called with a data pointer and a len parameter that tells us
>> how many bytes are pointed to by data. When invoking memcpy() we need
>> to careful to only copy len bytes.
>>
>> Previously we would copy TLV_LENGTH(len) bytes which would copy an extra
>> 4 bytes past the end of the data pointer which newer GCC versions
>> complain about.
>>
>>   In file included from test.c:17:
>>   In function 'TLV_SET',
>>       inlined from 'test' at test.c:186:5:
>>   /usr/include/linux/tipc_config.h:317:3:
>>   warning: 'memcpy' forming offset [33, 36] is out of the bounds [0, 32]
>>   of object 'bearer_name' with type 'char[32]' [-Warray-bounds]
>>       memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   test.c: In function 'test':
>>   test.c::161:10: note:
>>   'bearer_name' declared here
>>       char bearer_name[TIPC_MAX_BEARER_NAME];
>>            ^~~~~~~~~~~
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> 
> But now the pad bytes at the end are uninitialized.
> 
> The whole idea is that the encapsulating TLV object has to be rounded
> up in size based upon the given 'len' for the data.
> 

TLV_LENGTH() does not account for any padding bytes due to the 
alignment. TLV_SPACE() does but that wasn't used in the code before my 
change.

Are you suggesting something like this


-        if (len && data)
-               memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
+        if (len && data) {
+               memcpy(TLV_DATA(tlv_ptr), data, len);
+               memset(TLV_DATA(tlv_ptr) + len, 0, TLV_SPACE(len) - 
TLV_LENGTH(len));
+        }

Re: [PATCH] tipc: Avoid copying bytes beyond the supplied data

[Arvind Sankar]

On Sun, May 05, 2019 at 08:20:14PM +0000, Chris Packham wrote:
> On 4/05/19 4:45 PM, David Miller wrote:
> > From: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Date: Thu,  2 May 2019 15:10:04 +1200
> > 
> >> TLV_SET is called with a data pointer and a len parameter that tells us
> >> how many bytes are pointed to by data. When invoking memcpy() we need
> >> to careful to only copy len bytes.
> >>
> >> Previously we would copy TLV_LENGTH(len) bytes which would copy an extra
> >> 4 bytes past the end of the data pointer which newer GCC versions
> >> complain about.
> >>
> >>   In file included from test.c:17:
> >>   In function 'TLV_SET',
> >>       inlined from 'test' at test.c:186:5:
> >>   /usr/include/linux/tipc_config.h:317:3:
> >>   warning: 'memcpy' forming offset [33, 36] is out of the bounds [0, 32]
> >>   of object 'bearer_name' with type 'char[32]' [-Warray-bounds]
> >>       memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
> >>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>   test.c: In function 'test':
> >>   test.c::161:10: note:
> >>   'bearer_name' declared here
> >>       char bearer_name[TIPC_MAX_BEARER_NAME];
> >>            ^~~~~~~~~~~
> >>
> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > 
> > But now the pad bytes at the end are uninitialized.
> > 
> > The whole idea is that the encapsulating TLV object has to be rounded
> > up in size based upon the given 'len' for the data.
> > 
> 
> TLV_LENGTH() does not account for any padding bytes due to the 
> alignment. TLV_SPACE() does but that wasn't used in the code before my 
> change.
> 
> Are you suggesting something like this
> 
> 
> -        if (len && data)
> -               memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
> +        if (len && data) {
> +               memcpy(TLV_DATA(tlv_ptr), data, len);
> +               memset(TLV_DATA(tlv_ptr) + len, 0, TLV_SPACE(len) - 
> TLV_LENGTH(len));
> +        }
> 
> 

For zeroing out the padding, should that be done in TCM_SET in the same
file as well? That one only copies data_len bytes but doesn't zero out
any alignment padding.

Re: [PATCH] tipc: Avoid copying bytes beyond the supplied data

[Chris Packham]

On 6/05/19 8:57 AM, Arvind Sankar wrote:
> On Sun, May 05, 2019 at 08:20:14PM +0000, Chris Packham wrote:
>> On 4/05/19 4:45 PM, David Miller wrote:
>>> From: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> Date: Thu,  2 May 2019 15:10:04 +1200
>>>
>>>> TLV_SET is called with a data pointer and a len parameter that tells us
>>>> how many bytes are pointed to by data. When invoking memcpy() we need
>>>> to careful to only copy len bytes.
>>>>
>>>> Previously we would copy TLV_LENGTH(len) bytes which would copy an extra
>>>> 4 bytes past the end of the data pointer which newer GCC versions
>>>> complain about.
>>>>
>>>>    In file included from test.c:17:
>>>>    In function 'TLV_SET',
>>>>        inlined from 'test' at test.c:186:5:
>>>>    /usr/include/linux/tipc_config.h:317:3:
>>>>    warning: 'memcpy' forming offset [33, 36] is out of the bounds [0, 32]
>>>>    of object 'bearer_name' with type 'char[32]' [-Warray-bounds]
>>>>        memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
>>>>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>    test.c: In function 'test':
>>>>    test.c::161:10: note:
>>>>    'bearer_name' declared here
>>>>        char bearer_name[TIPC_MAX_BEARER_NAME];
>>>>             ^~~~~~~~~~~
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>
>>> But now the pad bytes at the end are uninitialized.
>>>
>>> The whole idea is that the encapsulating TLV object has to be rounded
>>> up in size based upon the given 'len' for the data.
>>>
>>
>> TLV_LENGTH() does not account for any padding bytes due to the
>> alignment. TLV_SPACE() does but that wasn't used in the code before my
>> change.
>>
>> Are you suggesting something like this
>>
>>
>> -        if (len && data)
>> -               memcpy(TLV_DATA(tlv_ptr), data, tlv_len);
>> +        if (len && data) {
>> +               memcpy(TLV_DATA(tlv_ptr), data, len);
>> +               memset(TLV_DATA(tlv_ptr) + len, 0, TLV_SPACE(len) -
>> TLV_LENGTH(len));
>> +        }
>>
>>
> 
> For zeroing out the padding, should that be done in TCM_SET in the same
> file as well? That one only copies data_len bytes but doesn't zero out
> any alignment padding.
> 

Thanks for pointing out TCM_SET it at least adds some weight to my 
TLV_SET change.

For both TLV_SET and TCM_SET both have the potential problem of SPACE - 
LENGTH bytes being uninitialised. TCM_SET additionally has some reserved 
bytes that aren't set either.

Both of these could be solved with a memset(msg, 0, SPACE(data_len)); at 
the start. I'm not sure what the performance impact of this would be but 
trying to figure out the difference between SPACE and LENGTH plus the 
pointer arithmetic wouldn't be free either.