Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC, WIP, v6 05/10] media: vidtv: add wrappers for memcpy and
 memset
To:     "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>,
        mchehab+huawei@kernel.org, sean@mess.org,
        kstewart@linuxfoundation.org, allison@lohutok.net,
        tglx@linutronix.de
Cc:     linux-media@vger.kernel.org,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-kernel@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20200520070334.1778751-1-dwlsalmeida@gmail.com>
 <20200520070334.1778751-6-dwlsalmeida@gmail.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <2b00fc7a-8818-b0be-7e42-a2df9787876b@linuxfoundation.org>
Date:   Mon, 1 Jun 2020 10:51:34 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200520070334.1778751-6-dwlsalmeida@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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