Re: [PATCH 2/3] media: replace strcpy() by strscpy()

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Mon, 10 Sep 2018 16:48:47 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> Em Mon, 10 Sep 2018 09:16:35 -0700
> Kees Cook <keescook@chromium.org> escreveu:
> 
> > On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab
> > <mchehab+samsung@kernel.org> 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(&ictx->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(&ictx->lock);