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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
