[PATCH V2] Fix buffer size warning for strcpy

[PATCH V2] Fix buffer size warning for strcpy

[Nigel Croxon]

To meet requirements of Common Criteria certification vulnerability
assessment. Static code analysis has been run and found the following
error:
buffer_size_warning: Calling "strncpy" with a maximum size
argument of 16 bytes on destination array "ve->name" of
size 16 bytes might leave the destination string unterminated.

The change is to make the destination size to fit the allocated size.

V2: Change from zero-terminated to zero-padded on memset and
change from using strncpy to memcpy, feedback from Neil Brown.

Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
---
 super-ddf.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index dc8e512..1771316 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -2637,9 +2637,13 @@ static int init_super_ddf_bvd(struct supertype *st,
 		ve->init_state = DDF_init_not;
 
 	memset(ve->pad1, 0xff, 14);
-	memset(ve->name, ' ', 16);
-	if (name)
-		strncpy(ve->name, name, 16);
+	memset(ve->name, '\0', sizeof(ve->name));
+	if (name) {
+		int l = strlen(ve->name);
+		if (l > 16)
+			l = 16;
+		memcpy(ve->name, name, l);
+	}
 	ddf->virt->populated_vdes =
 		cpu_to_be16(be16_to_cpu(ddf->virt->populated_vdes)+1);
 
-- 
2.29.2

Re: [PATCH V2] Fix buffer size warning for strcpy

[NeilBrown]

On Thu, 19 Aug 2021, Nigel Croxon wrote:
> To meet requirements of Common Criteria certification vulnerability
> assessment. Static code analysis has been run and found the following
> error:
> buffer_size_warning: Calling "strncpy" with a maximum size
> argument of 16 bytes on destination array "ve->name" of
> size 16 bytes might leave the destination string unterminated.
> 
> The change is to make the destination size to fit the allocated size.

You really should explain here why we change from filling with spaces to
filling with nuls.

> 
> V2: Change from zero-terminated to zero-padded on memset and
> change from using strncpy to memcpy, feedback from Neil Brown.
> 
> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
> ---
>  super-ddf.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index dc8e512..1771316 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -2637,9 +2637,13 @@ static int init_super_ddf_bvd(struct supertype *st,
>  		ve->init_state = DDF_init_not;
>  
>  	memset(ve->pad1, 0xff, 14);
> -	memset(ve->name, ' ', 16);
> -	if (name)
> -		strncpy(ve->name, name, 16);
> +	memset(ve->name, '\0', sizeof(ve->name));
> +	if (name) {
> +		int l = strlen(ve->name);
> +		if (l > 16)
> +			l = 16;
> +		memcpy(ve->name, name, l);
> +	}

Reviewed-by: NeilBrown <neilb@suse.de>

Thanks,
NeilBrown

>  	ddf->virt->populated_vdes =
>  		cpu_to_be16(be16_to_cpu(ddf->virt->populated_vdes)+1);
>  
> -- 
> 2.29.2
> 
> 

Re: [PATCH V2] Fix buffer size warning for strcpy

[Gal Ofri]

> You really should explain here why we change from filling with spaces to
> filling with nuls.
I guess that a commit-comment would still be needed, but I think that a
more conventional approach could make it all clearer:

- memset(ve->name, ' ', 16);
- if (name)
-	strncpy(ve->name, name, 16);
+ int l = strnlen(ve->name, 16);
+ memcpy(ve->name, name, l);
+ if (l < 16) /* no null-termination expected when all the space is used */
+ 	ve->name[l] = '\0';

Cheers,
Gal

Re: [PATCH V2] Fix buffer size warning for strcpy

[Gal Ofri]

> > You really should explain here why we change from filling with spaces to
> > filling with nuls.
> I guess that a commit-comment would still be needed, but I think that a
> more conventional approach could make it all clearer:
> 
> - memset(ve->name, ' ', 16);
> - if (name)
> -	strncpy(ve->name, name, 16);
> + int l = strnlen(ve->name, 16);
> + memcpy(ve->name, name, l);
> + if (l < 16) /* no null-termination expected when all the space is used */
> + 	ve->name[l] = '\0';
> 
> Cheers,
> Gal

my bad - I obviously meant strnlen(name, 16).

Re: [PATCH V2] Fix buffer size warning for strcpy

[NeilBrown]

On Mon, 23 Aug 2021, Gal Ofri wrote:
> > You really should explain here why we change from filling with spaces to
> > filling with nuls.
> I guess that a commit-comment would still be needed, but I think that a
> more conventional approach could make it all clearer:
> 
> - memset(ve->name, ' ', 16);
> - if (name)
> -	strncpy(ve->name, name, 16);
> + int l = strnlen(ve->name, 16);
> + memcpy(ve->name, name, l);
> + if (l < 16) /* no null-termination expected when all the space is used */
> + 	ve->name[l] = '\0';

No, that would be wrong.  It doesn't clear the whole array.

NeilBrown

Re: [PATCH V2] Fix buffer size warning for strcpy

[Tkaczyk, Mariusz]

On 19.08.2021 15:10, Nigel Croxon wrote:

> +	memset(ve->name, '\0', sizeof(ve->name));
> +	if (name) {
> +		int l = strlen(ve->name);
> +		if (l > 16)
> +			l = 16;
> +		memcpy(ve->name, name, l);
> +	}

What about:
if (name)
	/*
	 * Name might not be null terminated.
	 */
	strncpy(ve->name, name, sizeof(ve->name));
else
	memset(ve->name, '\0', sizeof(ve->name));

If size is less than sizeof(ve->name) then strncpy will automatically
fill rest with "\0".

Mariusz

Re: [PATCH V2] Fix buffer size warning for strcpy

[NeilBrown]

On Mon, 23 Aug 2021, Tkaczyk, Mariusz wrote:
> On 19.08.2021 15:10, Nigel Croxon wrote:
> 
> > +	memset(ve->name, '\0', sizeof(ve->name));
> > +	if (name) {
> > +		int l = strlen(ve->name);
> > +		if (l > 16)
> > +			l = 16;
> > +		memcpy(ve->name, name, l);
> > +	}
> 
> What about:
> if (name)
> 	/*
> 	 * Name might not be null terminated.
> 	 */
> 	strncpy(ve->name, name, sizeof(ve->name));

I really like the idea of using strncpy().  I didn't realize it would
nul-pad to the full size, and that is exactly what we want.
So

  strncpy(ve->name, name?:"", sizeof(ve->name));

would be a complete solution.

Thanks,
NeilBrown

Re: [PATCH V2] Fix buffer size warning for strcpy

[hillr]

On Tue Aug 24, 2021 at 08:46:49AM +1000, NeilBrown wrote:

> On Mon, 23 Aug 2021, Tkaczyk, Mariusz wrote:
> > On 19.08.2021 15:10, Nigel Croxon wrote:
> > 
> > > +	memset(ve->name, '\0', sizeof(ve->name));
> > > +	if (name) {
> > > +		int l = strlen(ve->name);
> > > +		if (l > 16)
> > > +			l = 16;
> > > +		memcpy(ve->name, name, l);
> > > +	}
> > 
> > What about:
> > if (name)
> > 	/*
> > 	 * Name might not be null terminated.
> > 	 */
> > 	strncpy(ve->name, name, sizeof(ve->name));
> 
> I really like the idea of using strncpy().  I didn't realize it would
> nul-pad to the full size, and that is exactly what we want.
> So
> 
>   strncpy(ve->name, name?:"", sizeof(ve->name));
> 
> would be a complete solution.
> 
Except that won't get rid of the buffer warning that was the point of
this patch:

buffer_size_warning: Calling "strncpy" with a maximum size
argument of 16 bytes on destination array "ve->name" of
size 16 bytes might leave the destination string unterminated.

Looking at the code, I don't think we're relying on the destination
string being null terminated anyway (if it's the full 16 bytes), so it's
not actually going to cause an issue, but we'll still be left with the
warning. Presumably using memcpy doesn't flag on this (as it then
doesn't know the value being copied is meant to be a string), which is
why that was being proposed.

Cheers.
    Robin