Re: [PATCH 3/3] media: replace strncpy() by strscpy()
On 09/10/2018 02:19 PM, Mauro Carvalho Chehab wrote:
> The strncpy() function is being deprecated upstream. Replace
> it by the safer strscpy().
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/dvb-frontends/as102_fe.c | 2 +-
> drivers/media/dvb-frontends/dib7000p.c | 3 ++-
> drivers/media/dvb-frontends/dib8000.c| 4 ++--
> drivers/media/dvb-frontends/dib9000.c| 6 --
> drivers/media/dvb-frontends/dvb-pll.c| 2 +-
> drivers/media/dvb-frontends/m88ds3103.c | 2 +-
> drivers/media/pci/bt8xx/dst.c| 3 ++-
> drivers/media/pci/mantis/mantis_i2c.c| 2 +-
> drivers/media/pci/saa7134/saa7134-go7007.c | 2 +-
> drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> drivers/media/platform/davinci/vpfe_capture.c| 2 +-
> drivers/media/platform/davinci/vpif_capture.c| 2 +-
> drivers/media/platform/davinci/vpif_display.c| 3 +--
> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
> drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
> drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +-
> drivers/media/platform/mx2_emmaprp.c | 4 ++--
> drivers/media/platform/s5p-g2d/g2d.c | 6 +++---
> drivers/media/platform/ti-vpe/vpe.c | 6 +++---
> drivers/media/platform/vicodec/vicodec-core.c| 4 ++--
> drivers/media/platform/vim2m.c | 4 ++--
> drivers/media/radio/si4713/si4713.c | 2 +-
> drivers/media/usb/go7007/go7007-usb.c| 16
> drivers/media/usb/go7007/go7007-v4l2.c | 2 +-
> drivers/media/usb/hdpvr/hdpvr-video.c| 9 +++--
> drivers/media/usb/pulse8-cec/pulse8-cec.c| 4 ++--
> drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 +-
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 ++--
> drivers/staging/media/bcm2048/radio-bcm2048.c| 4 ++--
> drivers/staging/media/imx/imx-ic-common.c| 2 +-
> drivers/staging/media/imx/imx-media-vdic.c | 2 +-
> drivers/staging/media/zoran/zoran_driver.c | 10 +-
> 32 files changed, 61 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/media/platform/davinci/vpfe_capture.c
> b/drivers/media/platform/davinci/vpfe_capture.c
> index ea3ddd5a42bd..6f094dea6bc2 100644
> --- a/drivers/media/platform/davinci/vpfe_capture.c
> +++ b/drivers/media/platform/davinci/vpfe_capture.c
> @@ -1759,7 +1759,7 @@ static int vpfe_probe(struct platform_device *pdev)
>
> mutex_lock(_lock);
>
> - strncpy(ccdc_cfg->name, vpfe_cfg->ccdc, 32);
> + strscpy(ccdc_cfg->name, vpfe_cfg->ccdc, 32);
32 -> sizeof(ccdc_cfg->name)
> /* Get VINT0 irq resource */
> res1 = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> if (!res1) {
> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> b/drivers/media/platform/davinci/vpif_capture.c
> index 62bced38db10..b246af6cc21f 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1254,7 +1254,7 @@ static int vpif_s_dv_timings(struct file *file, void
> *priv,
> } else {
> std_info->l5 = std_info->vsize - (bt->vfrontporch - 1);
> }
> - strncpy(std_info->name, "Custom timings BT656/1120", VPIF_MAX_NAME);
> + strscpy(std_info->name, "Custom timings BT656/1120", VPIF_MAX_NAME);
VPIF_MAX_NAME -> sizeof(std_info->name)
> std_info->width = bt->width;
> std_info->height = bt->height;
> std_info->frm_fmt = bt->interlaced ? 0 : 1;
> diff --git a/drivers/media/platform/davinci/vpif_display.c
> b/drivers/media/platform/davinci/vpif_display.c
> index 78eba66f4b2b..65f51ebef6b4 100644
> --- a/drivers/media/platform/davinci/vpif_display.c
> +++ b/drivers/media/platform/davinci/vpif_display.c
> @@ -987,8 +987,7 @@ static int vpif_s_dv_timings(struct file *file, void
> *priv,
> } else {
> std_info->l5 = std_info->vsize - (bt->vfrontporch - 1);
> }
> - strncpy(std_info->name, "Custom timings BT656/1120",
> - VPIF_MAX_NAME);
> + strscpy(std_info->name, "Custom timings BT656/1120", VPIF_MAX_NAME);
Ditto.
> std_info->width = bt->width;
> std_info->height = bt->height;
> std_info->frm_fmt = bt->interlaced ? 0 : 1;
> diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c
> b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> index f8d35e3ac1dc..9f4bca037b59 100644
> --- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
> +++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> @@ -615,7 +615,7 @@ static void vpu_init_ipi_handler(void *data, unsigned int
> len, void *priv)
> struct vpu_run *run = (struct vpu_run *)data;
>
> vpu->run.signaled = run->signaled;
> - strncpy(vpu->run.fw_ver, run->fw_ver, VPU_FW_VER_LEN);
> + strscpy(vpu->run.fw_ver, run->fw_ver, VPU_FW_VER_LEN);
VPU_FW_VER_LEN ->
Re: [PATCH 3/3] media: replace strncpy() by strscpy()
On Mon, Sep 10, 2018 at 11:34 AM, Mauro Carvalho Chehab wrote: > Em Mon, 10 Sep 2018 09:18:05 -0700 > Kees Cook escreveu: > >> On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab >> wrote: >> > The strncpy() function is being deprecated upstream. Replace >> > it by the safer strscpy(). >> >> This one I'm quite concerned about. This could lead to kernel memory >> exposures if any of the callers depend on strncpy()'s trailing >> NUL-padding to clear a buffer of prior contents. >> >> How did you validate that for these changes? > > That's actually easy for those familiar with the V4L2 API. There are > several fields at either uAPI or kAPI (or both) that have strings. > > For example, a video input has a name. > > So, for one familiar with the V4L2 API, it is clear that something > like: > > + strscpy(inp->name, zr->card.input[inp->index].name, > + sizeof(inp->name)); > > Is just filling the uAPI with the name of Input, with is, typically, > something like: > S-Video > Television > Radio > Composite > > A visual inspection of the patch shows that, on almost all cases, it is > either filling a device driver's name (used mainly for debug routines), > a video Input, a format description string, or the video caps fields > name and driver. It looks like the ioctl path also pre-clears the output buffer before handing it over to the per-driver routines, so I think this looks okay. It's a large patch, but if you're comfortable with it, go for it. :) -Kees -- Kees Cook Pixel Security
Re: [PATCH 3/3] media: replace strncpy() by strscpy()
Em Mon, 10 Sep 2018 09:18:05 -0700 Kees Cook escreveu: > On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab > wrote: > > The strncpy() function is being deprecated upstream. Replace > > it by the safer strscpy(). > > This one I'm quite concerned about. This could lead to kernel memory > exposures if any of the callers depend on strncpy()'s trailing > NUL-padding to clear a buffer of prior contents. > > How did you validate that for these changes? That's actually easy for those familiar with the V4L2 API. There are several fields at either uAPI or kAPI (or both) that have strings. For example, a video input has a name. So, for one familiar with the V4L2 API, it is clear that something like: + strscpy(inp->name, zr->card.input[inp->index].name, + sizeof(inp->name)); Is just filling the uAPI with the name of Input, with is, typically, something like: S-Video Television Radio Composite A visual inspection of the patch shows that, on almost all cases, it is either filling a device driver's name (used mainly for debug routines), a video Input, a format description string, or the video caps fields name and driver. Thanks, Mauro
Re: [PATCH 3/3] media: replace strncpy() by strscpy()
On Mon, Sep 10, 2018 at 5:19 AM, Mauro Carvalho Chehab wrote: > The strncpy() function is being deprecated upstream. Replace > it by the safer strscpy(). This one I'm quite concerned about. This could lead to kernel memory exposures if any of the callers depend on strncpy()'s trailing NUL-padding to clear a buffer of prior contents. How did you validate that for these changes? -Kees -- Kees Cook Pixel Security
[PATCH 3/3] media: replace strncpy() by strscpy()
The strncpy() function is being deprecated upstream. Replace it by the safer strscpy(). Signed-off-by: Mauro Carvalho Chehab --- drivers/media/dvb-frontends/as102_fe.c | 2 +- drivers/media/dvb-frontends/dib7000p.c | 3 ++- drivers/media/dvb-frontends/dib8000.c| 4 ++-- drivers/media/dvb-frontends/dib9000.c| 6 -- drivers/media/dvb-frontends/dvb-pll.c| 2 +- drivers/media/dvb-frontends/m88ds3103.c | 2 +- drivers/media/pci/bt8xx/dst.c| 3 ++- drivers/media/pci/mantis/mantis_i2c.c| 2 +- drivers/media/pci/saa7134/saa7134-go7007.c | 2 +- drivers/media/platform/am437x/am437x-vpfe.c | 2 +- drivers/media/platform/davinci/vpfe_capture.c| 2 +- drivers/media/platform/davinci/vpif_capture.c| 2 +- drivers/media/platform/davinci/vpif_display.c| 3 +-- drivers/media/platform/exynos4-is/fimc-capture.c | 2 +- drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +- drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +- drivers/media/platform/mx2_emmaprp.c | 4 ++-- drivers/media/platform/s5p-g2d/g2d.c | 6 +++--- drivers/media/platform/ti-vpe/vpe.c | 6 +++--- drivers/media/platform/vicodec/vicodec-core.c| 4 ++-- drivers/media/platform/vim2m.c | 4 ++-- drivers/media/radio/si4713/si4713.c | 2 +- drivers/media/usb/go7007/go7007-usb.c| 16 drivers/media/usb/go7007/go7007-v4l2.c | 2 +- drivers/media/usb/hdpvr/hdpvr-video.c| 9 +++-- drivers/media/usb/pulse8-cec/pulse8-cec.c| 4 ++-- drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 +- drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 ++-- drivers/staging/media/bcm2048/radio-bcm2048.c| 4 ++-- drivers/staging/media/imx/imx-ic-common.c| 2 +- drivers/staging/media/imx/imx-media-vdic.c | 2 +- drivers/staging/media/zoran/zoran_driver.c | 10 +- 32 files changed, 61 insertions(+), 61 deletions(-) diff --git a/drivers/media/dvb-frontends/as102_fe.c b/drivers/media/dvb-frontends/as102_fe.c index f59a102b0a64..9ba8f39fe310 100644 --- a/drivers/media/dvb-frontends/as102_fe.c +++ b/drivers/media/dvb-frontends/as102_fe.c @@ -467,7 +467,7 @@ struct dvb_frontend *as102_attach(const char *name, /* init frontend callback ops */ memcpy(>ops, _fe_ops, sizeof(struct dvb_frontend_ops)); - strncpy(fe->ops.info.name, name, sizeof(fe->ops.info.name)); + strscpy(fe->ops.info.name, name, sizeof(fe->ops.info.name)); return fe; diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c index 58387860b62d..d8cfdb4ce16e 100644 --- a/drivers/media/dvb-frontends/dib7000p.c +++ b/drivers/media/dvb-frontends/dib7000p.c @@ -2771,7 +2771,8 @@ static struct dvb_frontend *dib7000p_init(struct i2c_adapter *i2c_adap, u8 i2c_a dibx000_init_i2c_master(>i2c_master, DIB7000P, st->i2c_adap, st->i2c_addr); /* init 7090 tuner adapter */ - strncpy(st->dib7090_tuner_adap.name, "DiB7090 tuner interface", sizeof(st->dib7090_tuner_adap.name)); + strscpy(st->dib7090_tuner_adap.name, "DiB7090 tuner interface", + sizeof(st->dib7090_tuner_adap.name)); st->dib7090_tuner_adap.algo = _tuner_xfer_algo; st->dib7090_tuner_adap.algo_data = NULL; st->dib7090_tuner_adap.dev.parent = st->i2c_adap->dev.parent; diff --git a/drivers/media/dvb-frontends/dib8000.c b/drivers/media/dvb-frontends/dib8000.c index 3c3f8cb14845..286f8000b445 100644 --- a/drivers/media/dvb-frontends/dib8000.c +++ b/drivers/media/dvb-frontends/dib8000.c @@ -4458,8 +4458,8 @@ static struct dvb_frontend *dib8000_init(struct i2c_adapter *i2c_adap, u8 i2c_ad dibx000_init_i2c_master(>i2c_master, DIB8000, state->i2c.adap, state->i2c.addr); /* init 8096p tuner adapter */ - strncpy(state->dib8096p_tuner_adap.name, "DiB8096P tuner interface", - sizeof(state->dib8096p_tuner_adap.name)); + strscpy(state->dib8096p_tuner_adap.name, "DiB8096P tuner interface", + sizeof(state->dib8096p_tuner_adap.name)); state->dib8096p_tuner_adap.algo = _tuner_xfer_algo; state->dib8096p_tuner_adap.algo_data = NULL; state->dib8096p_tuner_adap.dev.parent = state->i2c.adap->dev.parent; diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c index 0183fb1346ef..c986687def30 100644 --- a/drivers/media/dvb-frontends/dib9000.c +++ b/drivers/media/dvb-frontends/dib9000.c @@ -2521,7 +2521,8 @@ struct dvb_frontend *dib9000_attach(struct i2c_adapter *i2c_adap, u8 i2c_addr, c dibx000_init_i2c_master(>i2c_master, DIB7000MC, st->i2c.i2c_adap, st->i2c.i2c_addr); st->tuner_adap.dev.parent = i2c_adap->dev.parent; - strncpy(st->tuner_adap.name, "DIB9000_FW TUNER ACCESS",
