| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2b00fc7a-8818-b0be-7e42-a2df9787876b@linuxfoundation.org>
Date: Mon, 1 Jun 2020 10:51:34 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: "Daniel W. S. Almeida" <dwlsalmeida@...il.com>,
mchehab+huawei@...nel.org, sean@...s.org,
kstewart@...uxfoundation.org, allison@...utok.net,
tglx@...utronix.de
Cc: linux-media@...r.kernel.org,
linux-kernel-mentees@...ts.linuxfoundation.org,
linux-kernel@...r.kernel.org,
Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [RFC, WIP, v6 05/10] media: vidtv: add wrappers for memcpy and
memset
Hi Daniel,
On 5/20/20 1:03 AM, Daniel W. S. Almeida wrote:
> From: "Daniel W. S. Almeida" <dwlsalmeida@...il.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@...il.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@...il.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@...il.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
Powered by blists - more mailing lists