From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> To: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> Cc: sean@mess.org, kstewart@linuxfoundation.org, allison@lohutok.net, tglx@linutronix.de, linux-media@vger.kernel.org, skhan@linuxfoundation.org, linux-kernel-mentees@lists.linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset Date: Sat, 2 May 2020 08:40:38 +0200 [thread overview] Message-ID: <20200502084038.07c38c4b@coco.lan> (raw) In-Reply-To: <20200502032216.197977-7-dwlsalmeida@gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> To: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> Cc: kstewart@linuxfoundation.org, sean@mess.org, linux-kernel@vger.kernel.org, tglx@linutronix.de, linux-kernel-mentees@lists.linuxfoundation.org, allison@lohutok.net, linux-media@vger.kernel.org Subject: Re: [Linux-kernel-mentees] [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset Date: Sat, 2 May 2020 08:40:38 +0200 [thread overview] Message-ID: <20200502084038.07c38c4b@coco.lan> (raw) In-Reply-To: <20200502032216.197977-7-dwlsalmeida@gmail.com> 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 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-05-02 6:40 UTC|newest] Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-02 3:22 [RFC, WIP, v4 00/11] media: vidtv: implement a virtual DVB driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 3:22 ` [RFC, WIP, v4 01/11] media: vidtv: add Kconfig entry Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 4:58 ` Mauro Carvalho Chehab 2020-05-02 4:58 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 02/11] media: vidtv: implement a tuner driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 5:27 ` Mauro Carvalho Chehab 2020-05-02 5:27 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 03/11] media: vidtv: implement a demodulator driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 5:58 ` Mauro Carvalho Chehab 2020-05-02 5:58 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 04/11] media: vidtv: move config structs into a separate header Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:02 ` Mauro Carvalho Chehab 2020-05-02 6:02 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 9:28 ` kbuild test robot 2020-05-02 3:22 ` [RFC, WIP, v4 05/11] media: vidtv: add a bridge driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:30 ` Mauro Carvalho Chehab 2020-05-02 6:30 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 21:12 ` Daniel W. S. Almeida 2020-05-02 21:12 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 10:05 ` kbuild test robot 2020-05-02 3:22 ` [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:40 ` Mauro Carvalho Chehab [this message] 2020-05-02 6:40 ` Mauro Carvalho Chehab 2020-05-03 7:06 ` Mauro Carvalho Chehab 2020-05-03 7:06 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 07/11] media: vidtv: add MPEG TS common code Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 7:09 ` Mauro Carvalho Chehab 2020-05-02 7:09 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 22:22 ` Daniel W. S. Almeida 2020-05-02 22:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 9:50 ` Mauro Carvalho Chehab 2020-05-03 9:50 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 08/11] media: vidtv: implement a PSI generator Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 7:51 ` Mauro Carvalho Chehab 2020-05-03 7:51 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 6:28 ` Daniel W. S. Almeida 2020-05-06 6:28 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 8:36 ` Mauro Carvalho Chehab 2020-05-06 8:36 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 09/11] media: vidtv: implement a PES packetizer Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 8:16 ` Mauro Carvalho Chehab 2020-05-03 8:16 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 6:55 ` Daniel W. S. Almeida 2020-05-06 6:55 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 8:59 ` Mauro Carvalho Chehab 2020-05-06 8:59 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 10/11] media: vidtv: Implement a SMPTE 302M encoder Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 8:57 ` Mauro Carvalho Chehab 2020-05-03 8:57 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 11/11] media: vidtv: Add a MPEG Transport Stream Multiplexer Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 9:41 ` kbuild test robot 2020-05-03 9:13 ` Mauro Carvalho Chehab 2020-05-03 9:13 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 7:05 ` Daniel W. S. Almeida 2020-05-06 7:05 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 9:01 ` Mauro Carvalho Chehab 2020-05-06 9:01 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200502084038.07c38c4b@coco.lan \ --to=mchehab+huawei@kernel.org \ --cc=allison@lohutok.net \ --cc=dwlsalmeida@gmail.com \ --cc=kstewart@linuxfoundation.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=sean@mess.org \ --cc=skhan@linuxfoundation.org \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.