Em Sat, 2 May 2020 00:22:11 -0300 "Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx> escreveu: > From: "Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx> > > 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@xxxxxxxxx> > --- > 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@xxxxxxxxx> > + */ > + > +#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@xxxxxxxxx> > + */ > + > +#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