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

[Rob Ward]

Hi,

On Wed, 3 Dec 2014, Arnd Bergmann wrote:

> 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.

Apologies, I will remove them in the future, I really should get 
send-email setup :-)
> 
> > 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.
> 
For the most part as a security consideration but a ever so 
slightly smaller kernel may be a nice side effect.

> 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.

The aim is to reduce the potential methods of attack, having access
to areas of memory via a device makes reading potentially important data 
from RAM very easy, this change doesn't stop this but does make it harder 
than opening a file.

Additionally many platforms(MIPS as an example) do not restict the area of 
RAM that can be accessed at all. /dev/mem works out which areas of memory 
can be read using the range_is_allowed() function in drivers/char/mem.c 
which in turn determines what can be read based on the config option 
CONFIG_STRICT_DEVMEM which if enabled calls devmem_is_allowed implemented
per platform.

devmem_is_allowed is not implemented on several platforms and the
default is to allow access to any location.

x86 and others do have restrictions implemented, usually based on
areas that bios places data etc.

On "secure" and embedded systems access to /dev/mem is not usually needed
and so disabling it is a desirable feature and prevents the need to 
implement custom restrictions.
> 
> > 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?

It would be nice to keep this in sync with DEVKMEM as
well, would a follow up change to implement this for kmem
to keep in sync be acceptable?

> 
> > 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.
>
I will modify to implement this way.
 
> >  #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.
> 
Will do

> 	Arnd
> 

Cheers,

Rob Ward
--
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/