| Message ID | 20200502032216.197977-7-dwlsalmeida@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vidtv: implement a virtual DVB driver | expand |
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
Em Sat, 2 May 2020 08:40:38 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > Em Sat, 2 May 2020 00:22:11 -0300 > "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu: > > > +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; > > +} When trying to use your memset wrapper, I noticed a few issues there. The first one is that you should not use __func__ directly at pr_* macros. Instead, just ensure that you have a pr_fmt() macro that ill be adding the driver's name and the function, e. g.: #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__ Besides that, the parameter order sounded weird: > > +u32 vidtv_memcpy(void *to, > > + const void *from, > > + size_t len, > > + u32 offset, > > + u32 buf_sz) The "to", "offset" and "buf_sz" arguments refer to the "to" buffer, while "from" and "len" refers to what will be copied from the "from" into the "to" buffer. Please re-order it, placing first the "to" args, then "from" arg, and finally the argument that applies to both, e. g.: size_t vidtv_memcpy(void *to, size_t to_offset, size_t to_size, const void *from, size_t len) (you should notice that I'm using size_t for all args there). The same is also valid for the memset. Finally, the places where this function is used require to pass twice the offset (from patch 08/11): > + nbytes += vidtv_memcpy(args.dest_buf + > + args.dest_offset + > + nbytes, > + &ts_header, > + sizeof(ts_header), > + args.dest_offset + nbytes, > + args.dest_buf_sz); That doesn't make much sense. I mean, if the arguments are "buf + offset", one would expect that the "buf" would be the head of a buffer, and the function would be adding the offset internally. So, the best would be to re-define it like: /** * vidtv_memcpy() - wrapper routine to be used by MPEG-TS * generator, in order to avoid going past the * output buffer. * @to: Starting element to where a MPEG-TS packet will * be copied. * @to_offset: Starting position of the @to buffer to be filled. * @to_size: Size of the @to buffer. * @from: Starting element of the buffer to be copied. * @ten: Number of elements to be copy from @from buffer * into @to+ @to_offset buffer. * * Note: * Real digital TV demod drivers should not have memcpy * wrappers. We use it here just because emulating a MPEG-TS * generation at kernelspace require some extra care. * * Return: * Returns the number of bytes */ size_t vidtv_memcpy(void *to, size_t to_offset, size_t to_size, const void *from, size_t len) { if unlikely(to_offset + len > to_size) { pr_err_ratelimited("overflow detected, skipping. Try increasing the buffer size\n"); return 0; } memcpy(to + to_offset, from, len); return len; } Thanks, Mauro
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; + } + + 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