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
[PATCH 2/3] media: replace strcpy() by strscpy()
The strcpy() function is being deprecated upstream. Replace it by the safer strscpy(). Signed-off-by: Mauro Carvalho Chehab --- drivers/media/common/saa7146/saa7146_video.c | 2 +- drivers/media/dvb-core/dvb_frontend.c | 2 +- drivers/media/dvb-frontends/mt312.c | 9 --- drivers/media/dvb-frontends/zl10039.c | 5 ++-- drivers/media/firewire/firedtv-fe.c | 2 +- drivers/media/i2c/ad5820.c| 2 +- drivers/media/i2c/lm3560.c| 3 ++- drivers/media/i2c/lm3646.c| 3 ++- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +- drivers/media/i2c/sr030pc30.c | 2 +- drivers/media/pci/bt8xx/bttv-driver.c | 4 +-- drivers/media/pci/cx23885/cx23885-417.c | 2 +- drivers/media/pci/cx23885/cx23885-alsa.c | 4 +-- drivers/media/pci/cx23885/cx23885-video.c | 11 drivers/media/pci/cx25821/cx25821-alsa.c | 8 +++--- drivers/media/pci/cx25821/cx25821-video.c | 6 ++--- drivers/media/pci/cx88/cx88-alsa.c| 6 ++--- drivers/media/pci/cx88/cx88-blackbird.c | 4 +-- drivers/media/pci/cx88/cx88-cards.c | 2 +- drivers/media/pci/cx88/cx88-video.c | 8 +++--- drivers/media/pci/dm1105/dm1105.c | 5 ++-- drivers/media/pci/dt3155/dt3155.c | 6 ++--- drivers/media/pci/meye/meye.c | 10 drivers/media/pci/ngene/ngene-i2c.c | 2 +- drivers/media/pci/pluto2/pluto2.c | 3 ++- drivers/media/pci/pt1/pt1.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 8 +++--- drivers/media/pci/saa7134/saa7134-i2c.c | 2 +- drivers/media/pci/saa7134/saa7134-video.c | 9 --- drivers/media/pci/saa7164/saa7164-core.c | 2 +- drivers/media/pci/saa7164/saa7164-encoder.c | 6 ++--- drivers/media/pci/saa7164/saa7164-vbi.c | 2 +- drivers/media/pci/smipcie/smipcie-main.c | 4 +-- drivers/media/pci/solo6x10/solo6x10-g723.c| 8 +++--- .../media/pci/solo6x10/solo6x10-v4l2-enc.c| 13 ++ drivers/media/pci/solo6x10/solo6x10-v4l2.c| 4 +-- drivers/media/pci/sta2x11/sta2x11_vip.c | 6 ++--- drivers/media/pci/ttpci/av7110_v4l.c | 2 +- drivers/media/pci/ttpci/budget-core.c | 3 ++- drivers/media/pci/tw5864/tw5864-video.c | 2 +- drivers/media/pci/tw68/tw68-video.c | 2 +- drivers/media/platform/am437x/am437x-vpfe.c | 3 ++- drivers/media/platform/atmel/atmel-isc.c | 6 ++--- drivers/media/platform/davinci/vpbe_display.c | 6 +++-- drivers/media/platform/davinci/vpbe_venc.c| 2 +- drivers/media/platform/davinci/vpif_capture.c | 6 +++-- drivers/media/platform/davinci/vpif_display.c | 3 ++- drivers/media/platform/fsl-viu.c | 8 +++--- .../media/platform/marvell-ccic/cafe-driver.c | 2 +- .../media/platform/marvell-ccic/mcam-core.c | 6 ++--- .../media/platform/soc_camera/soc_camera.c| 2 +- drivers/media/platform/via-camera.c | 6 ++--- drivers/media/platform/vivid/vivid-cec.c | 4 +-- drivers/media/platform/vivid/vivid-core.c | 4 +-- drivers/media/radio/dsbr100.c | 2 +- drivers/media/radio/radio-ma901.c | 2 +- drivers/media/radio/radio-mr800.c | 2 +- .../media/radio/si470x/radio-si470x-common.c | 2 +- drivers/media/radio/wl128x/fmdrv_v4l2.c | 4 +-- drivers/media/rc/imon.c | 4 +-- drivers/media/usb/au0828/au0828-video.c | 18 ++--- drivers/media/usb/cpia2/cpia2_v4l.c | 12 - drivers/media/usb/cx231xx/cx231xx-audio.c | 9 --- drivers/media/usb/cx231xx/cx231xx-video.c | 9 --- drivers/media/usb/em28xx/em28xx-audio.c | 8 +++--- drivers/media/usb/em28xx/em28xx-i2c.c | 3 ++- drivers/media/usb/em28xx/em28xx-video.c | 22 drivers/media/usb/hdpvr/hdpvr-video.c | 7 +++--- drivers/media/usb/pulse8-cec/pulse8-cec.c | 3 ++- drivers/media/usb/pwc/pwc-if.c| 2 +- drivers/media/usb/pwc/pwc-v4l.c | 2 +- .../media/usb/rainshadow-cec/rainshadow-cec.c | 3 ++- drivers/media/usb/stk1160/stk1160-i2c.c | 2 +- drivers/media/usb/stk1160/stk1160-v4l.c | 4 +-- drivers/media/usb/stkwebcam/stk-webcam.c | 21 ++-- drivers/media/usb/tm6000/tm6000-alsa.c| 6 ++--- drivers/media/usb/tm6000/tm6000-video.c | 6 ++--- .../media/usb/ttusb-budget/dvb-ttusb-budget.c | 3 ++- drivers/media/usb/usbvision/usbvision-video.c | 25 +++ drivers/media/usb/uvc/uvc_driver.c| 2 +- drivers/media/usb/zr364xx/zr364xx.c | 4 +-- .../media/davinci_vpfe/vpfe_mc_capture.c | 3 ++- drivers/staging/media/imx/imx6-mipi-csi2.c| 2 +- drivers/staging/media/zoran/zoran_card.c | 3 ++- 84 files changed, 240 insertions(+), 201 deletions(-)
