On Wed 04-11-26 18:47:22, Christian Brauner wrote:
> Every procfs file has a ->mmap since commit 786d7e1612f0 ("Fix
> rmmod/read/write races in /proc entries"), whether the entry has a
> ->proc_mmap or not. On NOMMU validate_mmap_request() takes that to mean
> the file can be mapped. procfs has no ->mmap_capabilities so a regular
> file gets NOMMU_MAP_COPY, and a private mapping never calls ->mmap at
> all. Instead do_mmap() takes nommu_region_sem for writing and
> do_mmap_private() reads the file into the copy with kernel_read().
>
> Reading /proc/maps takes nommu_region_sem as well:
>
> mmap(NULL, 4096, PROT_READ, MAP_PRIVATE, open("/proc/maps", O_RDONLY), 0)
>
> do_mmap() // down_write(&nommu_region_sem)
> -> do_mmap_private()
> -> kernel_read()
> -> seq_read_iter()
> -> nommu_region_list_start() // down_read()
>
> The task hangs for good, holding nommu_region_sem and its own mmap_lock,
> and every mmap(), munmap() and exit after that hangs behind it.
> /proc/maps is 0444 so any user can do that.
>
> Give the procfs files a ->mmap_capabilities on NOMMU that allows
> nothing. That covers every entry and not just /proc/maps, procfs can't
> know which entry's read takes nommu_region_sem. Mapping a procfs file
> then fails with -ENODEV, like mapping a file without ->mmap did before
> 786d7e1612f0. NOMMU never called ->proc_mmap() for a regular file
> anyway, procfs never offered NOMMU_MAP_DIRECT.
>
> Reported-by: Chris Mason <[email protected]>
> Fixes: 786d7e1612f0 ("Fix rmmod/read/write races in /proc entries")
> Cc: [email protected]
> Signed-off-by: Christian Brauner (Amutable) <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/proc/inode.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index b7634f975d98..9933f03ab9fb 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -466,6 +466,18 @@ proc_reg_get_unmapped_area(struct file *file, unsigned
> long orig_addr,
> return rv;
> }
>
> +#ifndef CONFIG_MMU
> +/*
> + * NOMMU never calls ->proc_mmap() for a regular file. It reads the file into
> + * a private copy instead, with nommu_region_sem held for writing, which
> + * reading /proc/maps takes as well. Don't map procfs files at all.
> + */
> +static unsigned int proc_reg_mmap_capabilities(struct file *file)
> +{
> + return 0;
> +}
> +#endif
> +
> static int proc_reg_open(struct inode *inode, struct file *file)
> {
> struct proc_dir_entry *pde = PDE(inode);
> @@ -559,6 +571,9 @@ static const struct file_operations proc_reg_file_ops = {
> .unlocked_ioctl = proc_reg_unlocked_ioctl,
> .mmap = proc_reg_mmap,
> .get_unmapped_area = proc_reg_get_unmapped_area,
> +#ifndef CONFIG_MMU
> + .mmap_capabilities = proc_reg_mmap_capabilities,
> +#endif
> .open = proc_reg_open,
> .release = proc_reg_release,
> };
> @@ -572,6 +587,9 @@ static const struct file_operations proc_iter_file_ops = {
> .unlocked_ioctl = proc_reg_unlocked_ioctl,
> .mmap = proc_reg_mmap,
> .get_unmapped_area = proc_reg_get_unmapped_area,
> +#ifndef CONFIG_MMU
> + .mmap_capabilities = proc_reg_mmap_capabilities,
> +#endif
> .open = proc_reg_open,
> .release = proc_reg_release,
> };
> @@ -586,6 +604,9 @@ static const struct file_operations
> proc_reg_file_ops_compat = {
> .compat_ioctl = proc_reg_compat_ioctl,
> .mmap = proc_reg_mmap,
> .get_unmapped_area = proc_reg_get_unmapped_area,
> +#ifndef CONFIG_MMU
> + .mmap_capabilities = proc_reg_mmap_capabilities,
> +#endif
> .open = proc_reg_open,
> .release = proc_reg_release,
> };
> @@ -600,6 +621,9 @@ static const struct file_operations
> proc_iter_file_ops_compat = {
> .compat_ioctl = proc_reg_compat_ioctl,
> .mmap = proc_reg_mmap,
> .get_unmapped_area = proc_reg_get_unmapped_area,
> +#ifndef CONFIG_MMU
> + .mmap_capabilities = proc_reg_mmap_capabilities,
> +#endif
> .open = proc_reg_open,
> .release = proc_reg_release,
> };
>
> --
> 2.53.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
