[RFC,WIP,v4,06/11] media: vidtv: add wrappers for memcpy and memset
Commit Message
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
Comments
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
@@ -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
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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