Re: [PATCH] drivers: char: mem: Make /dev/mem an optional device

[Arnd Bergmann]

On Wednesday 03 December 2014 08:21:04 Rob Ward wrote:
> From ecd3ac31f6be2372d2beac0cc4831e74aeae75d2 Mon Sep 17 00:00:00 2001
> From: Rob Ward <robert.ward114@xxxxxxxxxxxxxx>
> Date: Wed, 5 Nov 2014 19:13:53 +0000
> Subject: [PATCH] drivers: char: mem: Make /dev/mem an optional device

Please leave out these headers from the email. If you use git-send-email,
it will work the right way, but for regular email clients you need to
remove them manually.

> Adds Kconfig option CONFIG_DEVMEM that allows the
> /dev/mem device to be disabled.
> 
> Option defaults to /dev/mem enabled.

Please explain the purpose of this. Are you doing this for security
considerations, for space savings, or something else.

If you are worried about security, please explain the possible attack
vector you are closing, if you want to save kernel text size, show
how much you save. It also helps to show a guess of what might break
if you turn it off.

> Signed-off-by: Rob Ward <robert.ward114@xxxxxxxxxxxxxx>
> ---
>  drivers/char/Kconfig |  9 +++++++++
>  drivers/char/mem.c   | 19 ++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index efefd12..a4af822 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -6,6 +6,15 @@ menu "Character devices"
>  
>  source "drivers/tty/Kconfig"
>  
> +config DEVMEM
> +	bool "/dev/mem virtual device support"
> +	default y

Should this be

	bool "/dev/mem virtual device support" if EXPERT

so normal users don't accidentally disable it?

> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 524b707..feadc87 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -92,6 +92,7 @@ void __weak unxlate_dev_mem_ptr(unsigned long phys, void *addr)
>   * This funcion reads the *physical* memory. The f_pos points directly to the
>   * memory location.
>   */
> +#ifdef CONFIG_DEVMEM
>  static ssize_t read_mem(struct file *file, char __user *buf,
>  			size_t count, loff_t *ppos)
>  {
> @@ -216,6 +217,7 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
>  	*ppos += written;
>  	return written;
>  }
> +#endif

Adding a lot of #ifdef isn't nice, there are better ways to achieve the
same.

> +#ifdef CONFIG_DEVMEM
>  static const struct file_operations mem_fops = {
>  	.llseek		= memory_lseek,
>  	.read		= read_mem,
> @@ -720,6 +734,7 @@ static const struct file_operations mem_fops = {
>  	.open		= open_mem,
>  	.get_unmapped_area = get_unmapped_area_mem,
>  };
> +#endif

I would probably do it here an leave the mem_fops visible to the compiler
but mark it as '__maybe_unused' to avoid the warning when the reference below
is not there.

>  #ifdef CONFIG_DEVKMEM
>  static const struct file_operations kmem_fops = {
> @@ -782,7 +797,9 @@ static const struct memdev {
>  	const struct file_operations *fops;
>  	struct backing_dev_info *dev_info;
>  } devlist[] = {
> +#ifdef CONFIG_DEVMEM
>  	 [1] = { "mem", 0, &mem_fops, &directly_mappable_cdev_bdi },
> +#endif
>  #ifdef CONFIG_DEVKMEM
>  	 [2] = { "kmem", 0, &kmem_fops, &directly_mappable_cdev_bdi },
>  #endif

To keep it in sync with kmem, I would then submit a cleanup patch to
do the same for kmem_fops and replace the other #ifdef lines with
__maybe_unused statement for kmem_fops.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/