[PATCH 0/4] Static analysis fixes

[PATCH 0/4] Static analysis fixes

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

The asn1 fix is straightforward.

I'm not sure Denis likes strlcpy any more than strncpy, but given that
strncpy was being misused within ELL and a strlcpy is seen as a
preferred option in some circles, I added l_strlcpy to our utils.

Mat Martineau (4):
  asn1: Call va_end() before every return
  util: Add l_strlcpy
  unit: Add l_strlcpy tests
  genl: Fix termination of name strings

 ell/asn1-private.h |  4 +++-
 ell/genl.c         |  6 +++---
 ell/util.c         | 32 ++++++++++++++++++++++++++++++++
 ell/util.h         |  2 ++
 unit/test-util.c   | 41 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 81 insertions(+), 4 deletions(-)

-- 
2.14.1

[PATCH 1/4] asn1: Call va_end() before every return

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

One call to va_end() was missing on an error path.
---
 ell/asn1-private.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ell/asn1-private.h b/ell/asn1-private.h
index 7827355..4ba17ac 100644
--- a/ell/asn1-private.h
+++ b/ell/asn1-private.h
@@ -108,8 +108,10 @@ static inline const uint8_t *asn1_der_find_elem_by_path(const uint8_t *buf,
 
 		pos = va_arg(vl, int);
 
-		if (!buf || elem_tag != (pos == -1 ? tag : ASN1_ID_SEQUENCE))
+		if (!buf || elem_tag != (pos == -1 ? tag : ASN1_ID_SEQUENCE)) {
+			va_end(vl);
 			return NULL;
+		}
 	}
 
 	va_end(vl);
-- 
2.14.1

[PATCH 2/4] util: Add l_strlcpy

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

---
 ell/util.c | 32 ++++++++++++++++++++++++++++++++
 ell/util.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/ell/util.c b/ell/util.c
index 98916e5..2879f9f 100644
--- a/ell/util.c
+++ b/ell/util.c
@@ -443,6 +443,38 @@ LIB_EXPORT bool l_str_has_prefix(const char *str, const char *prefix)
 	return !strncmp(str, prefix, prefix_len);
 }
 
+/**
+ * l_strlcpy:
+ * @dst: Destination buffer for string
+ * @src: Source buffer containing null-terminated string to copy
+ * @len: Maximum destination buffer space to use
+ *
+ * Copies a string from the @src buffer to the @dst buffer, using no
+ * more than @len bytes in @dst. @dst is guaranteed to be
+ * null-terminated. The caller can determine if the copy was truncated by
+ * checking if the return value is greater than or equal to @len.
+ *
+ * Returns: The length of the @src string, not including the null
+ * terminator.
+ */
+LIB_EXPORT size_t l_strlcpy(char* dst, const char *src, size_t len)
+{
+	size_t src_len = strlen(src);
+
+	if (len) {
+		if (src_len < len) {
+			len = src_len + 1;
+		} else {
+			len -= 1;
+			dst[len] = '\0';
+		}
+
+		memcpy(dst, src, len);
+	}
+
+	return src_len;
+}
+
 /**
  * l_str_has_suffix:
  * @str: A string to be examined
diff --git a/ell/util.h b/ell/util.h
index a77c0b8..126a172 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -222,6 +222,8 @@ char *l_strjoinv(char **str_array, const char delim);
 bool l_str_has_prefix(const char *str, const char *prefix);
 bool l_str_has_suffix(const char *str, const char *suffix);
 
+size_t l_strlcpy(char* dst, const char *src, size_t len);
+
 char *l_util_hexstring(const unsigned char *buf, size_t len);
 unsigned char *l_util_from_hexstring(const char *str, size_t *out_len);
 
-- 
2.14.1

[PATCH 3/4] unit: Add l_strlcpy tests

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

---
 unit/test-util.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/unit/test-util.c b/unit/test-util.c
index 427c74a..d2b5496 100644
--- a/unit/test-util.c
+++ b/unit/test-util.c
@@ -74,6 +74,45 @@ static void test_has_suffix(const void *test_data)
 	assert(!l_str_has_suffix(suffix, str));
 }
 
+static void do_strlcpy(size_t dst_bytes, size_t src_bytes)
+{
+	/* A valid address is needed for the destination buffer
+	 * even if l_strlcpy is told to not copy anything there
+	 */
+	char *dst = l_malloc(dst_bytes ?: 1);
+	char *src = l_malloc(src_bytes);
+	size_t src_strlen = src_bytes - 1;
+
+	memset(dst, ' ', dst_bytes ?: 1);
+	memset(src, '@', src_strlen);
+	src[src_strlen] = '\0';
+
+	assert(l_strlcpy(dst, src, dst_bytes) == src_strlen);
+
+	if (!dst_bytes) {
+		assert(*dst == ' ');
+	} else if (src_strlen >= dst_bytes) {
+		/* Copy was truncated */
+		assert(strlen(dst) == dst_bytes - 1);
+		assert(l_str_has_prefix(src, dst));
+	} else
+		assert(!strcmp(src, dst));
+
+	l_free(dst);
+	l_free(src);
+}
+
+static void test_strlcpy(const void *test_data)
+{
+	do_strlcpy(0, 1);
+	do_strlcpy(0, 10);
+	do_strlcpy(10, 8);
+	do_strlcpy(10, 9);
+	do_strlcpy(10, 10);
+	do_strlcpy(10, 11);
+	do_strlcpy(10, 12);
+}
+
 int main(int argc, char *argv[])
 {
 	l_test_init(&argc, &argv);
@@ -83,5 +122,7 @@ int main(int argc, char *argv[])
 
 	l_test_add("l_util_has_suffix", test_has_suffix, NULL);
 
+	l_test_add("l_strlcpy", test_strlcpy, NULL);
+
 	return l_test_run();
 }
-- 
2.14.1

[PATCH 4/4] genl: Fix termination of name strings

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

The genl_mcast and l_genl_family structs both have name fields of length
GENL_NAMSIZ. They are expected to be null terminated strings, but
strncpy() was being misused and not terminating long names
correctly. Depending on the context, this would result in an
unterminated string being passed to strcmp() or a kernel warning.
---
 ell/genl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ell/genl.c b/ell/genl.c
index 0ab6c98..b41a174 100644
--- a/ell/genl.c
+++ b/ell/genl.c
@@ -156,7 +156,7 @@ static struct l_genl_family *family_alloc(struct l_genl *genl,
 	family = l_new(struct l_genl_family, 1);
 
 	family->genl = genl;
-	strncpy(family->name, name, GENL_NAMSIZ);
+	l_strlcpy(family->name, name, GENL_NAMSIZ);
 
 	family->op_list = l_queue_new();
 	family->mcast_list = l_queue_new();
@@ -232,7 +232,7 @@ static void family_add_mcast(struct l_genl_family *family, const char *name,
 
 	mcast = l_new(struct genl_mcast, 1);
 
-	strncpy(mcast->name, name, GENL_NAMSIZ);
+	l_strlcpy(mcast->name, name, GENL_NAMSIZ);
 	mcast->id = id;
 	mcast->users = 0;
 
@@ -1047,7 +1047,7 @@ static void get_family_callback(struct l_genl_msg *msg, void *user_data)
 			family->id = *((uint16_t *) data);
 			break;
 		case CTRL_ATTR_FAMILY_NAME:
-			strncpy(family->name, data, GENL_NAMSIZ);
+			l_strlcpy(family->name, data, GENL_NAMSIZ);
 			break;
 		case CTRL_ATTR_VERSION:
 			family->version = *((uint32_t *) data);
-- 
2.14.1

Re: [PATCH 2/4] util: Add l_strlcpy

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

Hi Mat,

On 09/22/2017 05:12 PM, Mat Martineau wrote:
> ---
>   ell/util.c | 32 ++++++++++++++++++++++++++++++++
>   ell/util.h |  2 ++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/ell/util.c b/ell/util.c
> index 98916e5..2879f9f 100644
> --- a/ell/util.c
> +++ b/ell/util.c
> @@ -443,6 +443,38 @@ LIB_EXPORT bool l_str_has_prefix(const char *str, const char *prefix)
>   	return !strncmp(str, prefix, prefix_len);
>   }
>   
> +/**
> + * l_strlcpy:
> + * @dst: Destination buffer for string
> + * @src: Source buffer containing null-terminated string to copy
> + * @len: Maximum destination buffer space to use
> + *
> + * Copies a string from the @src buffer to the @dst buffer, using no
> + * more than @len bytes in @dst. @dst is guaranteed to be
> + * null-terminated. The caller can determine if the copy was truncated by
> + * checking if the return value is greater than or equal to @len.
> + *
> + * Returns: The length of the @src string, not including the null
> + * terminator.
> + */
> +LIB_EXPORT size_t l_strlcpy(char* dst, const char *src, size_t len)
> +{
> +	size_t src_len = strlen(src);
> +
> +	if (len) {
> +		if (src_len < len) {
> +			len = src_len + 1;
> +		} else {
> +			len -= 1;
> +			dst[len] = '\0';
> +		}
> +
> +		memcpy(dst, src, len);

how does a direct for/while loop implementation compare speed wise to 
calling strlen & memcpy?

> +	}
> +
> +	return src_len;
> +}
> +
>   /**
>    * l_str_has_suffix:
>    * @str: A string to be examined
> diff --git a/ell/util.h b/ell/util.h
> index a77c0b8..126a172 100644
> --- a/ell/util.h
> +++ b/ell/util.h
> @@ -222,6 +222,8 @@ char *l_strjoinv(char **str_array, const char delim);
>   bool l_str_has_prefix(const char *str, const char *prefix);
>   bool l_str_has_suffix(const char *str, const char *suffix);
>   
> +size_t l_strlcpy(char* dst, const char *src, size_t len);
> +
>   char *l_util_hexstring(const unsigned char *buf, size_t len);
>   unsigned char *l_util_from_hexstring(const char *str, size_t *out_len);
>   
> 

Regards,
-Denis

Re: [PATCH 1/4] asn1: Call va_end() before every return

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 249 bytes --]

Hi Mat,

On 09/22/2017 05:12 PM, Mat Martineau wrote:
> One call to va_end() was missing on an error path.
> ---
>   ell/asn1-private.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 4/4] genl: Fix termination of name strings

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

Hi Mat,

On 09/22/2017 05:12 PM, Mat Martineau wrote:
> The genl_mcast and l_genl_family structs both have name fields of length
> GENL_NAMSIZ. They are expected to be null terminated strings, but
> strncpy() was being misused and not terminating long names
> correctly. Depending on the context, this would result in an
> unterminated string being passed to strcmp() or a kernel warning.
> ---
>   ell/genl.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ell/genl.c b/ell/genl.c
> index 0ab6c98..b41a174 100644
> --- a/ell/genl.c
> +++ b/ell/genl.c
> @@ -156,7 +156,7 @@ static struct l_genl_family *family_alloc(struct l_genl *genl,
>   	family = l_new(struct l_genl_family, 1);
>   
>   	family->genl = genl;
> -	strncpy(family->name, name, GENL_NAMSIZ);
> +	l_strlcpy(family->name, name, GENL_NAMSIZ);

It would seem that l_genl_family_new should sanitize the name.  There's 
no point in truncating a name silently and trying to use it afterwards 
just to fail spectacularly anyway.

>   
>   	family->op_list = l_queue_new();
>   	family->mcast_list = l_queue_new();
> @@ -232,7 +232,7 @@ static void family_add_mcast(struct l_genl_family *family, const char *name,
>   
>   	mcast = l_new(struct genl_mcast, 1);
>   
> -	strncpy(mcast->name, name, GENL_NAMSIZ);
> +	l_strlcpy(mcast->name, name, GENL_NAMSIZ);
>   	mcast->id = id;
>   	mcast->users = 0;
>   
> @@ -1047,7 +1047,7 @@ static void get_family_callback(struct l_genl_msg *msg, void *user_data)
>   			family->id = *((uint16_t *) data);
>   			break;
>   		case CTRL_ATTR_FAMILY_NAME:
> -			strncpy(family->name, data, GENL_NAMSIZ);
> +			l_strlcpy(family->name, data, GENL_NAMSIZ);
>   			break;
>   		case CTRL_ATTR_VERSION:
>   			family->version = *((uint32_t *) data);
> 

The other two are not called with user input but get their values from 
the kernel, no?

Regards,
-Denis

Re: [PATCH 2/4] util: Add l_strlcpy

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]


Hi Denis,

On Mon, 25 Sep 2017, Denis Kenzior wrote:

> Hi Mat,
>
> On 09/22/2017 05:12 PM, Mat Martineau wrote:
>> ---
>>   ell/util.c | 32 ++++++++++++++++++++++++++++++++
>>   ell/util.h |  2 ++
>>   2 files changed, 34 insertions(+)
>> 
>> diff --git a/ell/util.c b/ell/util.c
>> index 98916e5..2879f9f 100644
>> --- a/ell/util.c
>> +++ b/ell/util.c
>> @@ -443,6 +443,38 @@ LIB_EXPORT bool l_str_has_prefix(const char *str, 
>> const char *prefix)
>>   	return !strncmp(str, prefix, prefix_len);
>>   }
>>   +/**
>> + * l_strlcpy:
>> + * @dst: Destination buffer for string
>> + * @src: Source buffer containing null-terminated string to copy
>> + * @len: Maximum destination buffer space to use
>> + *
>> + * Copies a string from the @src buffer to the @dst buffer, using no
>> + * more than @len bytes in @dst. @dst is guaranteed to be
>> + * null-terminated. The caller can determine if the copy was truncated by
>> + * checking if the return value is greater than or equal to @len.
>> + *
>> + * Returns: The length of the @src string, not including the null
>> + * terminator.
>> + */
>> +LIB_EXPORT size_t l_strlcpy(char* dst, const char *src, size_t len)
>> +{
>> +	size_t src_len = strlen(src);
>> +
>> +	if (len) {
>> +		if (src_len < len) {
>> +			len = src_len + 1;
>> +		} else {
>> +			len -= 1;
>> +			dst[len] = '\0';
>> +		}
>> +
>> +		memcpy(dst, src, len);
>
> how does a direct for/while loop implementation compare speed wise to calling 
> strlen & memcpy?

It's about twice as fast to use strlen/memcpy. The direct 
implementation I used for comparison:

         const char *orig_src = src;

         if (len) {
                 while (*src != '\0' && --len) {
                         *dst++ = *src++;
                 }
                 *dst = '\0';
         }
         return (src - orig_src) + strlen(src);

I modified the unit test to run a bunch of iterations with some more 
string sizes and to take performance samples during the strlcpy test. 
gperftools showed that the direct code spent 57.39% of execution time 
inside l_strlcpy and the strlen/memcpy technique spent 16.06% of execution 
time inside l_strlcpy. Turning off the profiling stuff and just using 
'time', direct took 53.973 seconds and strlen/memcpy took 25.176 seconds 
of user time.

>
>> +	}
>> +
>> +	return src_len;
>> +}
>> +
>>   /**
>>    * l_str_has_suffix:
>>    * @str: A string to be examined
>> diff --git a/ell/util.h b/ell/util.h
>> index a77c0b8..126a172 100644
>> --- a/ell/util.h
>> +++ b/ell/util.h
>> @@ -222,6 +222,8 @@ char *l_strjoinv(char **str_array, const char delim);
>>   bool l_str_has_prefix(const char *str, const char *prefix);
>>   bool l_str_has_suffix(const char *str, const char *suffix);
>>   +size_t l_strlcpy(char* dst, const char *src, size_t len);
>> +
>>   char *l_util_hexstring(const unsigned char *buf, size_t len);
>>   unsigned char *l_util_from_hexstring(const char *str, size_t *out_len);
>> 
>
> Regards,
> -Denis
>

--
Mat Martineau
Intel OTC

Re: [PATCH 2/4] util: Add l_strlcpy

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Hi Mat,

>>
>> how does a direct for/while loop implementation compare speed wise to 
>> calling strlen & memcpy?
> 
> It's about twice as fast to use strlen/memcpy. The direct implementation 
> I used for comparison:
> 
>          const char *orig_src = src;
> 
>          if (len) {
>                  while (*src != '\0' && --len) {
>                          *dst++ = *src++;
>                  }
>                  *dst = '\0';
>          }
>          return (src - orig_src) + strlen(src);
> 
> I modified the unit test to run a bunch of iterations with some more 
> string sizes and to take performance samples during the strlcpy test. 
> gperftools showed that the direct code spent 57.39% of execution time 
> inside l_strlcpy and the strlen/memcpy technique spent 16.06% of 
> execution time inside l_strlcpy. Turning off the profiling stuff and 
> just using 'time', direct took 53.973 seconds and strlen/memcpy took 
> 25.176 seconds of user time.
> 

Interesting, seems counter-intuitive since we're walking the string 
twice.  I went ahead and applied the original patch 2 + 3.  Much 
appreciated!

Regards,
-Denis

Re: [PATCH 4/4] genl: Fix termination of name strings

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2397 bytes --]


Hi Denis -

On Mon, 25 Sep 2017, Denis Kenzior wrote:

> Hi Mat,
>
> On 09/22/2017 05:12 PM, Mat Martineau wrote:
>> The genl_mcast and l_genl_family structs both have name fields of length
>> GENL_NAMSIZ. They are expected to be null terminated strings, but
>> strncpy() was being misused and not terminating long names
>> correctly. Depending on the context, this would result in an
>> unterminated string being passed to strcmp() or a kernel warning.
>> ---
>>   ell/genl.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/ell/genl.c b/ell/genl.c
>> index 0ab6c98..b41a174 100644
>> --- a/ell/genl.c
>> +++ b/ell/genl.c
>> @@ -156,7 +156,7 @@ static struct l_genl_family *family_alloc(struct l_genl 
>> *genl,
>>   	family = l_new(struct l_genl_family, 1);
>>     	family->genl = genl;
>> -	strncpy(family->name, name, GENL_NAMSIZ);
>> +	l_strlcpy(family->name, name, GENL_NAMSIZ);
>
> It would seem that l_genl_family_new should sanitize the name.  There's no 
> point in truncating a name silently and trying to use it afterwards just to 
> fail spectacularly anyway.

How about checking for truncation here in family_alloc using the return 
value of l_strlcpy? family_alloc could return NULL, and l_genl_family_new 
already knows what to do with that.

>
>>     	family->op_list = l_queue_new();
>>   	family->mcast_list = l_queue_new();
>> @@ -232,7 +232,7 @@ static void family_add_mcast(struct l_genl_family 
>> *family, const char *name,
>>     	mcast = l_new(struct genl_mcast, 1);
>>   -	strncpy(mcast->name, name, GENL_NAMSIZ);
>> +	l_strlcpy(mcast->name, name, GENL_NAMSIZ);
>>   	mcast->id = id;
>>   	mcast->users = 0;
>>   @@ -1047,7 +1047,7 @@ static void get_family_callback(struct l_genl_msg 
>> *msg, void *user_data)
>>   			family->id = *((uint16_t *) data);
>>   			break;
>>   		case CTRL_ATTR_FAMILY_NAME:
>> -			strncpy(family->name, data, GENL_NAMSIZ);
>> +			l_strlcpy(family->name, data, GENL_NAMSIZ);
>>   			break;
>>   		case CTRL_ATTR_VERSION:
>>   			family->version = *((uint32_t *) data);
>> 
>
> The other two are not called with user input but get their values from the 
> kernel, no?

Mostly (one use of family_add_mcast uses a hard-coded string). The current 
use of strncpy is incorrect no matter where the name comes from.

--
Mat Martineau
Intel OTC

Re: [PATCH 4/4] genl: Fix termination of name strings

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2255 bytes --]

Hi Mat,

>>> -    strncpy(family->name, name, GENL_NAMSIZ); >>> +    l_strlcpy(family->name, name, GENL_NAMSIZ);
>>
>> It would seem that l_genl_family_new should sanitize the name.  
>> There's no point in truncating a name silently and trying to use it 
>> afterwards just to fail spectacularly anyway.
> 
> How about checking for truncation here in family_alloc using the return 
> value of l_strlcpy? family_alloc could return NULL, and 
> l_genl_family_new already knows what to do with that.
> 

But then we're allocating a family object, memsetting it, etc 
unnecessarily, no?

>>
>>>         family->op_list = l_queue_new();
>>>       family->mcast_list = l_queue_new();
>>> @@ -232,7 +232,7 @@ static void family_add_mcast(struct l_genl_family 
>>> *family, const char *name,
>>>         mcast = l_new(struct genl_mcast, 1);
>>>   -    strncpy(mcast->name, name, GENL_NAMSIZ);
>>> +    l_strlcpy(mcast->name, name, GENL_NAMSIZ);
>>>       mcast->id = id;
>>>       mcast->users = 0;
>>>   @@ -1047,7 +1047,7 @@ static void get_family_callback(struct 
>>> l_genl_msg *msg, void *user_data)
>>>               family->id = *((uint16_t *) data);
>>>               break;
>>>           case CTRL_ATTR_FAMILY_NAME:
>>> -            strncpy(family->name, data, GENL_NAMSIZ);
>>> +            l_strlcpy(family->name, data, GENL_NAMSIZ);
>>>               break;
>>>           case CTRL_ATTR_VERSION:
>>>               family->version = *((uint32_t *) data);
>>>
>>
>> The other two are not called with user input but get their values from 
>> the kernel, no?
> 
> Mostly (one use of family_add_mcast uses a hard-coded string). The 
> current use of strncpy is incorrect no matter where the name comes from.
> 

why is it incorrect?  We're getting the name from the kernel and copying 
it into our own data structure.  If the kernel uses GENL_NAMSIZ max-size 
buffers, then using strncpy is just fine, no?

At least I don't see how strlcpy is saving you here.  If you want to be 
paranoid, you'd be taking the tlv length value into account as well.

Regards,
-Denis

Re: [PATCH 4/4] genl: Fix termination of name strings

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3385 bytes --]


On Mon, 25 Sep 2017, Denis Kenzior wrote:

> Hi Mat,
>
>>>> -    strncpy(family->name, name, GENL_NAMSIZ); >>> + 
>>>> l_strlcpy(family->name, name, GENL_NAMSIZ);
>>> 
>>> It would seem that l_genl_family_new should sanitize the name.  There's no 
>>> point in truncating a name silently and trying to use it afterwards just 
>>> to fail spectacularly anyway.
>> 
>> How about checking for truncation here in family_alloc using the return 
>> value of l_strlcpy? family_alloc could return NULL, and l_genl_family_new 
>> already knows what to do with that.
>> 
>
> But then we're allocating a family object, memsetting it, etc unnecessarily, 
> no?

Yes, in the very rare case of a programmer error during development. Are 
we that worried about the microsecond-level efficiency of a very rarely 
executed error path?

If family_alloc uses strcpy, then you depend on reviewers giving 
family_alloc the same level of scrutiny they do strcpy. You can't depend 
on static analysis to catch it in the future after flagging it as "ok" 
now.

If family_alloc uses l_strlcpy, then the common case gets an extra call to 
strlen.

>>>
>>>>         family->op_list = l_queue_new();
>>>>       family->mcast_list = l_queue_new();
>>>> @@ -232,7 +232,7 @@ static void family_add_mcast(struct l_genl_family 
>>>> *family, const char *name,
>>>>         mcast = l_new(struct genl_mcast, 1);
>>>>   -    strncpy(mcast->name, name, GENL_NAMSIZ);
>>>> +    l_strlcpy(mcast->name, name, GENL_NAMSIZ);
>>>>       mcast->id = id;
>>>>       mcast->users = 0;
>>>>   @@ -1047,7 +1047,7 @@ static void get_family_callback(struct l_genl_msg 
>>>> *msg, void *user_data)
>>>>               family->id = *((uint16_t *) data);
>>>>               break;
>>>>           case CTRL_ATTR_FAMILY_NAME:
>>>> -            strncpy(family->name, data, GENL_NAMSIZ);
>>>> +            l_strlcpy(family->name, data, GENL_NAMSIZ);
>>>>               break;
>>>>           case CTRL_ATTR_VERSION:
>>>>               family->version = *((uint32_t *) data);
>>>> 
>>> 
>>> The other two are not called with user input but get their values from the 
>>> kernel, no?
>> 
>> Mostly (one use of family_add_mcast uses a hard-coded string). The current 
>> use of strncpy is incorrect no matter where the name comes from.
>> 
>
> why is it incorrect?  We're getting the name from the kernel and copying it 
> into our own data structure.  If the kernel uses GENL_NAMSIZ max-size 
> buffers, then using strncpy is just fine, no?

It's incorrect because you aren't guaranteed a null-terminated destination 
buffer. It would be correct to use strncpy with GENL_NAMSIZ-1 and write a 
'\0' to to the end of the buffer if it's not known to be zeroed.

>
> At least I don't see how strlcpy is saving you here.  If you want to be 
> paranoid, you'd be taking the tlv length value into account as well.

It's making the current code work as intended.

strlcpy guarantees two things: that you don't write past the end of your 
destination buffer (corrupting other data) while copying, and that the 
destination buffer is null-terminated after the copy. strncpy does the 
former, but not the latter.

--
Mat Martineau
Intel OTC

Re: [PATCH 4/4] genl: Fix termination of name strings

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

Hi Mat,
> 
> Yes, in the very rare case of a programmer error during development. Are 
> we that worried about the microsecond-level efficiency of a very rarely 
> executed error path?

Not really about performance, no.  But readability wise its far easier 
to just error out early than tear things down.

In this particular case it doesn't matter much.  And by the way 
family_alloc result isn't checked inside l_genl_new...

> 
> If family_alloc uses strcpy, then you depend on reviewers giving 
> family_alloc the same level of scrutiny they do strcpy. You can't depend 
> on static analysis to catch it in the future after flagging it as "ok" now.
> 
> If family_alloc uses l_strlcpy, then the common case gets an extra call 
> to strlen.

Fundamentally this feels like a hammer seeking a nail.  Can we make the 
tool work for us and not the other way around?  If the tool can't figure 
out that the string length has been sanitized previously, what good is it?

>>
>> why is it incorrect?  We're getting the name from the kernel and 
>> copying it into our own data structure.  If the kernel uses 
>> GENL_NAMSIZ max-size buffers, then using strncpy is just fine, no?
> 
> It's incorrect because you aren't guaranteed a null-terminated 
> destination buffer. It would be correct to use strncpy with 
> GENL_NAMSIZ-1 and write a '\0' to to the end of the buffer if it's not 
> known to be zeroed.
> 

So the kernel is misbehaving...

>>
>> At least I don't see how strlcpy is saving you here.  If you want to 
>> be paranoid, you'd be taking the tlv length value into account as well.
> 
> It's making the current code work as intended.
> 

If you don't trust the kernel and we really want to be paranoid, we 
should use the length value returned by l_genl_attr_next and check that 
its <= GENL_NAMESIZ, etc.

Regards,
-Denis