diff mbox series

[RFC,WIP,v6,05/10] media: vidtv: add wrappers for memcpy and memset

Message ID 20200520070334.1778751-6-dwlsalmeida@gmail.com (mailing list archive)
State New, archived
Headers show
Series media: vidtv: implement a virtual DVB driver | expand

Commit Message

Daniel Almeida May 20, 2020, 7:03 a.m. UTC 
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

Comments

Shuah Khan June 1, 2020, 4:51 p.m. UTC | #1 
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
diff mbox series

Patch

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)) {
+		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