lkml.org 
[lkml]   [2020]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC, WIP, v6 05/10] media: vidtv: add wrappers for memcpy and memset
From
Date
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

\
 
 \ /
  Last update: 2020-06-01 18:52    [W:0.141 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site