Em Mon, 10 Sep 2018 16:48:47 -0300
Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx> escreveu:
> Em Mon, 10 Sep 2018 09:16:35 -0700
> Kees Cook <keescook@xxxxxxxxxxxx> escreveu:
>
> > On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab
> > <mchehab+samsung@xxxxxxxxxx> 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);
