On 2013年04月26日 22:34, Dan Carpenter wrote: > 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. > If the API would define the 'nd_ps_desc' must be "a NUL terminated string", strlcpy() was OK, and strncpy would be incorrect. > 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. > OK, thanks for your information. > 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. > Yes, when API has already defined and used, we'd better not to change it, except we have to change for solving issues. So if it is really an issue which related with API (caused by API), we have to change the API. > 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. > If so, I think one of correct fixing patch is just like my 'diff' above: "use strlcpy instead of strncpy, and still keep memset() alive" ;-) And also let nd_ps_desc in the nd_struct struct changed to MAX_DESC_LEN to avoid this confusion down the road (just what Bill said) Thanks. > regards, > dan carpenter > > -- Chen Gang Asianux Corporation _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
