lkml.org 
[lkml]   [2020]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset
    Em Sat,  2 May 2020 00:22:11 -0300
    "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

    > From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>
    >
    > A lot of code in this driver is for serializing structures. This is
    > error prone.
    >
    > Therefore, prevent buffer overflows by wrapping memcpy and memset,
    > comparing the requested length against the buffer size.
    >
    > Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com>
    > ---
    > drivers/media/test-drivers/vidtv/Makefile | 3 ++
    > .../media/test-drivers/vidtv/vidtv_common.c | 44 +++++++++++++++++++
    > .../media/test-drivers/vidtv/vidtv_common.h | 28 ++++++++++++
    > 3 files changed, 75 insertions(+)
    > create mode 100644 drivers/media/test-drivers/vidtv/vidtv_common.c
    > create mode 100644 drivers/media/test-drivers/vidtv/vidtv_common.h
    >
    > diff --git a/drivers/media/test-drivers/vidtv/Makefile b/drivers/media/test-drivers/vidtv/Makefile
    > index a9f1993dd5443..9ea9485d42189 100644
    > --- a/drivers/media/test-drivers/vidtv/Makefile
    > +++ b/drivers/media/test-drivers/vidtv/Makefile
    > @@ -1,3 +1,6 @@
    > # SPDX-License-Identifier: GPL-2.0
    >
    > +vidtv_demod-objs := vidtv_common.o
    > +vidtv_bridge-objs := vidtv_common.o
    > +
    > obj-$(CONFIG_DVB_VIDTV) += vidtv_tuner.o vidtv_demod.o vidtv_bridge.o
    > diff --git a/drivers/media/test-drivers/vidtv/vidtv_common.c b/drivers/media/test-drivers/vidtv/vidtv_common.c
    > new file mode 100644
    > index 0000000000000..28f10630499a9
    > --- /dev/null
    > +++ b/drivers/media/test-drivers/vidtv/vidtv_common.c
    > @@ -0,0 +1,44 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +/*
    > + * The Virtual DVB test driver serves as a reference DVB driver and helps
    > + * validate the existing APIs in the media subsystem. It can also aid
    > + * developers working on userspace applications.
    > + *
    > + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com>
    > + */
    > +
    > +#include <linux/types.h>
    > +#include <linux/string.h>
    > +#include <linux/printk.h>
    > +
    > +u32 vidtv_memcpy(void *to,
    > + const void *from,
    > + size_t len,
    > + u32 offset,
    > + u32 buf_sz)
    > +{
    > + if (buf_sz && offset + len > buf_sz) {
    > + pr_err("%s: overflow detected, skipping. Try increasing the buffer size",
    > + __func__);
    > + return 0;

    shouldn't it return an error?

    > + }
    > +
    > + memcpy(to, from, len);
    > + return len;
    > +}
    > +
    > +u32 vidtv_memset(void *to,
    > + int c,
    > + size_t len,
    > + u32 offset,
    > + u32 buf_sz)
    > +{
    > + if (buf_sz && offset + len > buf_sz) {
    > + pr_err("%s: overflow detected, skipping. Try increasing the buffer size",
    > + __func__);
    > + return 0;
    > + }
    > +
    > + memset(to, c, len);
    > + return len;
    > +}
    > diff --git a/drivers/media/test-drivers/vidtv/vidtv_common.h b/drivers/media/test-drivers/vidtv/vidtv_common.h
    > new file mode 100644
    > index 0000000000000..64072c010dc66
    > --- /dev/null
    > +++ b/drivers/media/test-drivers/vidtv/vidtv_common.h
    > @@ -0,0 +1,28 @@
    > +/* SPDX-License-Identifier: GPL-2.0 */
    > +/*
    > + * The Virtual DVB test driver serves as a reference DVB driver and helps
    > + * validate the existing APIs in the media subsystem. It can also aid
    > + * developers working on userspace applications.
    > + *
    > + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com>
    > + */
    > +
    > +#ifndef VIDTV_COMMON_H
    > +#define VIDTV_COMMON_H
    > +
    > +#include <linux/types.h>
    > +#include <media/dvb_frontend.h>
    > +
    > +u32 vidtv_memcpy(void *to,
    > + const void *from,
    > + size_t len,
    > + u32 offset,
    > + u32 buf_sz);
    > +
    > +u32 vidtv_memset(void *to,
    > + int c,
    > + size_t len,
    > + u32 offset,
    > + u32 buf_sz);
    > +
    > +#endif // VIDTV_COMMON_H


    On a generic note, I don't like seeing functions or macros like those
    re-defining existing Kernel functions like memcpy(), memset(), etc.

    This is actually a very common pattern when vendors try to submit
    new drivers upstream: several of them have a generic code, and use an
    OS-specific abstraction layer, with lots of defines, inline functions
    and re-definitions for Kernel functions.

    Before upstreaming a driver (or removing one from staging), the driver
    should get rid of those.

    On **this very specific case**, I see the value of having it there, as
    you're not doing it as a normal Digital TV driver, but, instead, using
    those in order to emulate an MPEG-TS encoding.

    Yet, as this driver is meant to be a sort of "tutorial" for ones
    implementing such features, please add a WARNING at both the header and
    at the source code, saying that normal drivers should not do that,
    explaining why, in this specific case (where you're simulating a MPEG-TS
    in software) it makes sense to have such functions.

    Thanks,
    Mauro

    \
     
     \ /
      Last update: 2020-05-02 08:41    [W:5.397 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site