Messages in this thread | | | Subject | Re: [RFC, WIP, v6 05/10] media: vidtv: add wrappers for memcpy and memset | From | Shuah Khan <> | Date | Mon, 1 Jun 2020 10:51:34 -0600 |
| |
Hi Daniel,
On 5/20/20 1:03 AM, Daniel W. S. Almeida wrote: > 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> > --- > .../media/test-drivers/vidtv/vidtv_common.c | 86 +++++++++++++++++++ > .../media/test-drivers/vidtv/vidtv_common.h | 27 ++++++ > 2 files changed, 113 insertions(+) > create mode 100644 drivers/media/test-drivers/vidtv/vidtv_common.c > create mode 100644 drivers/media/test-drivers/vidtv/vidtv_common.h >
I don't see these added to Makefile. Building the driver with the patch has no effect.
In any case I don't see any value in adding the wrappers here. They really do nothing other than doing range checks. It adda a layer of indirection that is unnecessary.
> 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..6810212087c17 > --- /dev/null > +++ b/drivers/media/test-drivers/vidtv/vidtv_common.c > @@ -0,0 +1,86 @@ > +// 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> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ":%s, %d: " fmt, __func__, __LINE__ > + > +#include <linux/printk.h> > +#include <linux/ratelimit.h> > +#include <linux/string.h> > +#include <linux/types.h> > + > +#include "vidtv_common.h" > + > +/** > + * 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. > + * @len: 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 because emulating MPEG-TS > + * generation at kernelspace requires some extra care. > + * > + * Return: > + * Returns the number of bytes written > + */ > +u32 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)) {
Use of unlikely isn't necessarily beneficial in all cases.
> + pr_err_ratelimited("overflow detected, skipping. Try increasing the buffer size\n"); > + return 0; > + } > + > + memcpy(to + to_offset, from, len); > + return len; > +} > + > +/** > + * vidtv_memset() - wrapper routine to be used by MPEG-TS > + * generator, in order to avoid going past the > + * output buffer. > + * @to: Starting element to set > + * @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 memset > + * wrappers. We use it here because emulating MPEG-TS > + * generation at kernelspace requires some extra care. > + * > + * Return: > + * Returns the number of bytes written > + */ > +u32 vidtv_memset(void *to, > + size_t to_offset, > + size_t to_size, > + const int c, > + size_t len) > +{ > + if (unlikely(to_offset + len > to_size)) { > + pr_err_ratelimited("overflow detected, skipping. Try increasing the buffer size\n"); > + return 0; > + } > + > + memset(to + to_offset, 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..a3cb303cc7423 > --- /dev/null > +++ b/drivers/media/test-drivers/vidtv/vidtv_common.h > @@ -0,0 +1,27 @@ > +/* 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> > + > +u32 vidtv_memcpy(void *to, > + size_t to_offset, > + size_t to_size, > + const void *from, > + size_t len); > + > +u32 vidtv_memset(void *to, > + size_t to_offset, > + size_t to_size, > + int c, > + size_t len); > + > +#endif // VIDTV_COMMON_H >
thanks, -- Shuah
| |