Re: [PATCH 2/3] media: replace strcpy() by strscpy()
Em Mon, 10 Sep 2018 16:48:47 -0300 Mauro Carvalho Chehab escreveu: > Em Mon, 10 Sep 2018 09:16:35 -0700 > Kees Cook escreveu: > > > On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab > > wrote: > > > The strcpy() function is being deprecated upstream. Replace > > > it by the safer strscpy(). > > > > Did you verify that all the destination buffers here are arrays and > > not pointers? For example: > > > > struct thing { > > char buffer[64]; > > char *ptr; > > } > > > > strscpy(instance->buffer, source, sizeof(instance->buffer)); > > > > is correct. > > > > But: > > > > strscpy(instance->ptr, source, sizeof(instance->ptr)); > > > > will not be and will truncate strings to sizeof(char *). > > > > If you _did_ verify this, I'd love to know more about your tooling. :) > > I ended by implementing a simple tooling to test... it found just > one place where it was wrong. I'll send the correct patch. Btw, at the only case it was trying to fill a pointer was for some sysfs fill. AFAIKT, the buffer size there is PAGE_SIZE, so, I guess the enclosed patch would be the right way to use strscpy(). Yet, IMHO, a better fix would be if the parameters for DEVICE_ATTR store field would have a count. Thanks, Mauro diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c index 1041c056854d..989d2554ec72 100644 --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -772,9 +772,9 @@ static ssize_t show_associate_remote(struct device *d, mutex_lock(>lock); if (ictx->rf_isassociating) - strcpy(buf, "associating\n"); + strscpy(buf, "associating\n", PAGE_SIZE); else - strcpy(buf, "closed\n"); + strscpy(buf, "closed\n", PAGE_SIZE); dev_info(d, "Visit http://www.lirc.org/html/imon-24g.html for instructions on how to associate your iMON 2.4G DT/LT remote\n"); mutex_unlock(>lock);
Re: [PATCH 2/3] media: replace strcpy() by strscpy()
Em Mon, 10 Sep 2018 09:16:35 -0700 Kees Cook escreveu: > On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab > wrote: > > The strcpy() function is being deprecated upstream. Replace > > it by the safer strscpy(). > > Did you verify that all the destination buffers here are arrays and > not pointers? For example: > > struct thing { > char buffer[64]; > char *ptr; > } > > strscpy(instance->buffer, source, sizeof(instance->buffer)); > > is correct. > > But: > > strscpy(instance->ptr, source, sizeof(instance->ptr)); > > will not be and will truncate strings to sizeof(char *). > > If you _did_ verify this, I'd love to know more about your tooling. :) I ended by implementing a simple tooling to test... it found just one place where it was wrong. I'll send the correct patch. The tooling is actually a hack... see enclosed. Basically, it defines a __strscpy() that will try to create a negative array if the size is equal to a pointer size. Then, I replaced all occurrences of strscpy with __strcpy() with: $ for i in $(git grep -l strscpy drivers/media drivers/staging/media); do sed s,strscpy,__strscpy,g -i $i; done and compiled on 32 bits (that's my usual build). As all strings at the media API are bigger than 4 bytes, it will only complain if it tries to do a sizeof(*). Thanks, Mauro TEST hack diff --git a/include/linux/string.h b/include/linux/string.h index 4a5a0eb7df51..06a87e328293 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -66,6 +66,15 @@ extern char * strrchr(const char *,int); #endif extern char * __must_check skip_spaces(const char *); + +#define __strscpy(origin, dest, size) \ +({ \ + char zzz[1 - 2*(size == sizeof(char *))]; \ + zzz[0] = 1; \ + if (zzz[0] >2) zzz[0]++; \ + strscpy(origin, dest, size); \ +}) + extern char *strim(char *); static inline __must_check char *strstrip(char *str)
Re: [PATCH 2/3] media: replace strcpy() by strscpy()
On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab wrote: > The strcpy() function is being deprecated upstream. Replace > it by the safer strscpy(). Did you verify that all the destination buffers here are arrays and not pointers? For example: struct thing { char buffer[64]; char *ptr; } strscpy(instance->buffer, source, sizeof(instance->buffer)); is correct. But: strscpy(instance->ptr, source, sizeof(instance->ptr)); will not be and will truncate strings to sizeof(char *). If you _did_ verify this, I'd love to know more about your tooling. :) -Kees -- Kees Cook Pixel Security