[PATCH] staging: for dgrp, nd_ps_desc buffer length need be 'MAX_DESC_LEN + 1'

Dan Carpenter dan.carpenter at oracle.com
Fri Apr 26 14:34:45 UTC 2013


On Fri, Apr 26, 2013 at 04:40:43PM +0800, Chen Gang wrote:
> On 2013年04月26日 15:43, Dan Carpenter wrote:
> > On Wed, Apr 24, 2013 at 11:18:03AM +0800, Chen Gang wrote:
> >> > 
> >> > For dgrp, the buffer length of 'nd->nd_ps_desc' is 'MAX_DESC_LEN + 1', so
> >> > the buffer length of 'getnode.nd_ps_desc' also need be 'MAX_DESC_LEN + 1',
> >> > then can fully copy from 'nd->nd_ps_desc' to 'getnode.nd_ps_desc'.
> >> > 
> > This is a user space change so someone needs to check that it
> > doesn't break anything.
> 
> Oh, really it is.
> 
> It seem the next fixing depends on the API definition between user mode
> and kernel mode. For compatible for user mode, I prefer we do not touch
> the API (although the API is intended, not defined explicitly).
> 
> If the API defines the 'nd_ps_desc' as "a NUL terminated string except
> when the string length is MAX_DESC_LEN", the original is OK (I guess
> the original 'nd->nd_ps_desc' use 'MAX_DESC_LEN + 1', so can be sure
> the string always NUL terminated in kernel mode)
> 
> If the API defines the 'nd_ps_desc' must be "a NUL terminated string",
> I prefer the fix below rather than my original one:
> 
> ----------------------------diff begin----------------------------------
> 
> diff --git a/drivers/staging/dgrp/dgrp_dpa_ops.c b/drivers/staging/dgrp/dgrp_dpa_ops.c
> index d99c227..f4b7f6f 100644
> --- a/drivers/staging/dgrp/dgrp_dpa_ops.c
> +++ b/drivers/staging/dgrp/dgrp_dpa_ops.c
> @@ -391,7 +391,7 @@ static long dgrp_dpa_ioctl(struct file *file, unsigned int cmd,
>  		getnode.nd_rx_byte = nd->nd_rx_byte;
>  
>  		memset(&getnode.nd_ps_desc, 0, MAX_DESC_LEN);
> -		strncpy(getnode.nd_ps_desc, nd->nd_ps_desc, MAX_DESC_LEN);
> +		strlcpy(getnode.nd_ps_desc, nd->nd_ps_desc, MAX_DESC_LEN);

strlcpy() is wrong because it NULL terminates so it will truncate
the last character away.  strncpy() is matches the API description,
and btw it pads the rest of the buffer with NUL characters so the
memset is not needed.

I hate these "mostly NUL terminated" strings.  MAX_DESC_LEN
is 100 characters so it's very rarely going to be filled all the
way.  The special case where ->nd_ps_desc isn't terminated will
never be tested.

When I said that we have to be careful about changing the API, I
didn't mean that we can't or shouldn't do it.  I just meant that we
have to be careful not to break existing programs.

It seems likely to me that changing this struct is going to break
things.  But maybe we could change the other side and say that the
desc can be 99 characters and a NUL maximum.  Probably no one will
notice a change like that.

regards,
dan carpenter



More information about the devel mailing list