On Sat, Mar 11, 2017 at 09:47:30PM +0100, Julia Lawall wrote: > > > On Sun, 12 Mar 2017, simran singhal wrote: > > > Replace strcpy with strlcpy as strcpy does not check for buffer > > overflow. > > This is found using Flawfinder. > > > > Signed-off-by: simran singhal <singhalsimr...@gmail.com> > > --- > > drivers/staging/android/ashmem.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/android/ashmem.c > > b/drivers/staging/android/ashmem.c > > index 7cbad0d..eb2f4ef 100644 > > --- a/drivers/staging/android/ashmem.c > > +++ b/drivers/staging/android/ashmem.c > > @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void > > __user *name) > > if (unlikely(asma->file)) > > ret = -EINVAL; > > else > > - strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name); > > + strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name, > > + sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN)); > > There is a parenthesis in the wrong place.
Worse - moving parenthesis to just after asma->name would result in interestingly bogus value (size + amount skipped instead of size - amount skipped). Folks, blind changes in name of security are seriously counterproductive; fortunately, in this particular case overflow prevention is taken care of by earlier code (source of strcpy is a local array of size that isn't enough to cause trouble and it is NUL-terminated), so that particular strlcpy() is simply pointless, but if not for that... Variant with sizeof(asma->name) + ASHMEM_NAME_PREFIX_LEN would've invited an overflow *and* made it harder to spot in the future - "it uses strlcpy, no worries about overflows here"...
