[PATCH 00/14] Make the user mode driver code a better citizen

[Alexei Starovoitov]

On Sat, Jun 27, 2020 at 10:57:10PM +0900, Tetsuo Handa wrote:
> On 2020/06/27 21:59, Eric W. Biederman wrote:
> > Can you try replacing the __fput_sync with:
> > 	fput(file);
> >         flush_delayed_fput();
> >         task_work_run();
> 
> With below change, TOMOYO can obtain pathname like "tmpfs:/my\040test\040driver".
> 
> Please avoid WARN_ON() if printk() is sufficient (for friendliness to panic_on_warn=1 environments).
> For argv[], I guess that fork_usermode_driver() should receive argv[] as argument rather than
> trying to split info->driver_name, for somebody might want to pass meaningful argv[] (and
> TOMOYO wants to use meaningful argv[] as a hint for identifying the intent).
> 
> diff --git a/kernel/umd.c b/kernel/umd.c
> index de2f542191e5..ae6e85283f13 100644
> --- a/kernel/umd.c
> +++ b/kernel/umd.c
> @@ -7,6 +7,7 @@
>  #include <linux/mount.h>
>  #include <linux/fs_struct.h>
>  #include <linux/umd.h>
> +#include <linux/task_work.h>
>  
>  static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *name)
>  {
> @@ -25,7 +26,7 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
>  	if (IS_ERR(mnt))
>  		return mnt;
>  
> -	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY, 0700);
> +	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY | O_EXCL, 0700);
>  	if (IS_ERR(file)) {
>  		mntput(mnt);
>  		return ERR_CAST(file);
> @@ -41,23 +42,33 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
>  		return ERR_PTR(err);
>  	}
>  
> -	__fput_sync(file);
> +	if (current->flags & PF_KTHREAD) {
> +		__fput_sync(file);
> +	} else {
> +		fput(file);
> +		flush_delayed_fput();
> +		task_work_run();
> +	}

Thanks. This makes sense to me.

>  	return mnt;
>  }
>  
>  /**
>   * umd_load_blob - Remember a blob of bytes for fork_usermode_driver
> - * @info: information about usermode driver
> - * @data: a blob of bytes that can be executed as a file
> - * @len:  The lentgh of the blob
> + * @info: information about usermode driver (shouldn't be NULL)
> + * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
> + * @len:  The lentgh of the blob (shouldn't be 0)
>   *
>   */
>  int umd_load_blob(struct umd_info *info, const void *data, size_t len)
>  {
>  	struct vfsmount *mnt;
>  
> -	if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
> +	if (!info || !info->driver_name || !data || !len)
> +		return -EINVAL;
> +	if (info->wd.dentry || info->wd.mnt) {
> +		pr_info("%s already loaded.\n", info->driver_name);
>  		return -EBUSY;
> +	}

But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.

I would like to generalize elf_header_check() a bit and call it
before doing blob_to_mnt() to make sure that all blobs are elf files only.
Supporting '#!/bin/bash' or other things as blobs seems wrong to me.