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

From: Chris Packham
Date: Wed May 22 2019 - 16:49:08 EST


Hi Ying,

On 22/05/19 7:58 PM, Ying Xue wrote:
> On 5/20/19 11:45 AM, Chris Packham wrote:
>> 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];
>> ^~~~~~~~~~~
>>
>> We still want to ensure any padding bytes at the end are initialised, do
>> this with a explicit memset() rather than copy bytes past the end of
>> data. Apply the same logic to TCM_SET.
>>
>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>
> Acked-by: Ying Xue <ying.xue@xxxxxxxxxxxxx>
>
>
> But please make the same changes in usr/include/linux/tipc_config.h
>

I don't understand this comment. There is no usr/include in the kernel
source.

On most distros that is generated from include/uapi in the kernel source
and packaged as part of libc or a kernel-headers package. So once this
patch is accepted and makes it into the distros
/usr/include/linux/tipc_config.h will have this fix.

Adding Cc: stable might help get it there sooner. But I wanted to get it
reviewed/accepted first.

>> ---
>>
>> Changes in v2:
>> - Ensure padding bytes are initialised in both TLV_SET and TCM_SET
>>
>> include/uapi/linux/tipc_config.h | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/uapi/linux/tipc_config.h b/include/uapi/linux/tipc_config.h
>> index 4b2c93b1934c..4955e1a9f1bc 100644
>> --- a/include/uapi/linux/tipc_config.h
>> +++ b/include/uapi/linux/tipc_config.h
>> @@ -307,8 +307,10 @@ static inline int TLV_SET(void *tlv, __u16 type, void *data, __u16 len)
>> tlv_ptr = (struct tlv_desc *)tlv;
>> tlv_ptr->tlv_type = htons(type);
>> tlv_ptr->tlv_len = htons(tlv_len);
>> - 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_len);
>> + }
>> return TLV_SPACE(len);
>> }
>>
>> @@ -405,8 +407,10 @@ static inline int TCM_SET(void *msg, __u16 cmd, __u16 flags,
>> tcm_hdr->tcm_len = htonl(msg_len);
>> tcm_hdr->tcm_type = htons(cmd);
>> tcm_hdr->tcm_flags = htons(flags);
>> - if (data_len && data)
>> + if (data_len && data) {
>> memcpy(TCM_DATA(msg), data, data_len);
>> + memset(TCM_DATA(msg) + data_len, 0, TCM_SPACE(data_len) - msg_len);
>> + }
>> return TCM_SPACE(data_len);
>> }
>>
>>
>