Re: [PATCH] binfmt_misc: add two-steps registration (opt-in)

[Domenico Andreoli <domenico.andreoli@linux.com>]

Hi Eric and Kees,

On Tue, Mar 01, 2022 at 02:28:22PM +0100, Domenico Andreoli wrote:
> From: Domenico Andreoli <domenico.andreoli@linux.com>
> 
> Experimenting with new interpreter configurations can lead to annoying
> failures, when the system is left unable to load ELF binaries power
> cycling is the only way to get it back operational.
> 
> This patch tries to mitigate such conditions by adding an opt-in
> two-steps registration.
> 
> A new optional field is added to the configuration string, it's an
> expiration interval for the newly added interpreter. If the user is
> not able to confirm in time, possibly because the system is broken,
> the new interpreter is automatically disabled.

I was wondering whether, maybe, likely, you missed this patch of mine.

It would be great if you could just ack (or nack) it for later.

Thanks!
Domenico

> 
> Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> Cc: Kees Cook <keescook@chromium.org>
> 
> ---
>  Documentation/admin-guide/binfmt-misc.rst |   12 +++
>  fs/binfmt_misc.c                          |  112 ++++++++++++++++++++++++++++--
>  2 files changed, 113 insertions(+), 11 deletions(-)
> 
> Index: b/Documentation/admin-guide/binfmt-misc.rst
> ===================================================================
> --- a/Documentation/admin-guide/binfmt-misc.rst
> +++ b/Documentation/admin-guide/binfmt-misc.rst
> @@ -16,8 +16,8 @@ First you must mount binfmt_misc::
>  	mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc
>  
>  To actually register a new binary type, you have to set up a string looking like
> -``:name:type:offset:magic:mask:interpreter:flags`` (where you can choose the
> -``:`` upon your needs) and echo it to ``/proc/sys/fs/binfmt_misc/register``.
> +``:name:type:offset:magic:mask:interpreter:flags:timeout`` (where you can choose
> +the ``:`` upon your needs) and echo it to ``/proc/sys/fs/binfmt_misc/register``.
>  
>  Here is what the fields mean:
>  
> @@ -88,6 +88,14 @@ Here is what the fields mean:
>  	    emulation is installed and uses the opened image to spawn the
>  	    emulator, meaning it is always available once installed,
>  	    regardless of how the environment changes.
> +- ``timeout``
> +  is an optional field; the newly added interpreter is automatically
> +  disabled after the specified number of seconds. To cancel such
> +  count down, cat or echo something to ``/proc/.../the_name``.  This
> +  registration in two steps allows recovering a system left unusable
> +  by some wrong configuration. A timeout of 0 seconds effectively adds
> +  a disabled interpreter.  Values smaller than 0 or bigger than 120
> +  are invalid.
>  
>  
>  There are some restrictions:
> Index: b/fs/binfmt_misc.c
> ===================================================================
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -27,6 +27,8 @@
>  #include <linux/syscalls.h>
>  #include <linux/fs.h>
>  #include <linux/uaccess.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
>  
>  #include "internal.h"
>  
> @@ -49,6 +51,8 @@ enum {Enabled, Magic};
>  #define MISC_FMT_CREDENTIALS (1 << 29)
>  #define MISC_FMT_OPEN_FILE (1 << 28)
>  
> +struct node_timer;
> +
>  typedef struct {
>  	struct list_head list;
>  	unsigned long flags;		/* type, status, etc. */
> @@ -60,8 +64,15 @@ typedef struct {
>  	char *name;
>  	struct dentry *dentry;
>  	struct file *interp_file;
> +	struct node_timer *auto_disable;
> +	spinlock_t auto_disable_lock;
>  } Node;
>  
> +struct node_timer {
> +	struct timer_list timer;
> +	Node *node;
> +};
> +
>  static DEFINE_RWLOCK(entries_lock);
>  static struct file_system_type bm_fs_type;
>  static struct vfsmount *bm_mnt;
> @@ -69,19 +80,30 @@ static int entry_count;
>  
>  /*
>   * Max length of the register string.  Determined by:
> - *  - 7 delimiters
> - *  - name:   ~50 bytes
> - *  - type:   1 byte
> - *  - offset: 3 bytes (has to be smaller than BINPRM_BUF_SIZE)
> - *  - magic:  128 bytes (512 in escaped form)
> - *  - mask:   128 bytes (512 in escaped form)
> - *  - interp: ~50 bytes
> - *  - flags:  5 bytes
> + *  - 8 delimiters
> + *  - name:    ~50 bytes
> + *  - type:    1 byte
> + *  - offset:  3 bytes (has to be smaller than BINPRM_BUF_SIZE)
> + *  - magic:   128 bytes (512 in escaped form)
> + *  - mask:    128 bytes (512 in escaped form)
> + *  - interp:  ~50 bytes
> + *  - flags:   5 bytes
> + *  - timeout: 3 bytes
>   * Round that up a bit, and then back off to hold the internal data
>   * (like struct Node).
>   */
>  #define MAX_REGISTER_LENGTH 1920
>  
> +#define MAX_AUTO_DISABLE_TIMEOUT 120
> +
> +static void auto_disable_timer_fn(struct timer_list *t)
> +{
> +	struct node_timer *timer = container_of(t, struct node_timer, timer);
> +
> +	clear_bit(Enabled, &timer->node->flags);
> +	pr_info("%s: auto-disabled\n", timer->node->name);
> +}
> +
>  /*
>   * Check if we support the binfmt
>   * if we do, return the node, else NULL
> @@ -266,6 +288,41 @@ static char *check_special_flags(char *s
>  	return p;
>  }
>  
> +static char *setup_auto_disable(char *p, char *endp, Node *e)
> +{
> +	unsigned int timeout;
> +	char buf[4] = {0};
> +
> +	while (endp[-1] == '\n')
> +		endp--;
> +	if (p >= endp || *p != ':' || ++p == endp)
> +		return p;
> +
> +	endp = min(endp, p + sizeof(buf) - 1);
> +	memcpy(buf, p, (size_t) (endp - p));
> +
> +	if (kstrtouint(buf, 10, &timeout) || timeout > MAX_AUTO_DISABLE_TIMEOUT) {
> +		pr_info("%s: invalid timeout: %s\n", e->name, buf);
> +		return p;
> +	}
> +
> +	if (timeout == 0) {
> +		e->flags &= ~(1 << Enabled);
> +		return endp;
> +	}
> +
> +	e->auto_disable = kmalloc(sizeof(struct node_timer), GFP_KERNEL);
> +	if (!e->auto_disable)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pr_info("%s: auto-disable in %u seconds\n", e->name, timeout);
> +
> +	timer_setup(&e->auto_disable->timer, auto_disable_timer_fn, 0);
> +	e->auto_disable->timer.expires = jiffies + timeout * HZ;
> +	e->auto_disable->node = e;
> +	return endp;
> +}
> +
>  /*
>   * This registers a new binary format, it recognises the syntax
>   * ':name:type:offset:magic:mask:interpreter:flags'
> @@ -273,7 +330,7 @@ static char *check_special_flags(char *s
>   */
>  static Node *create_entry(const char __user *buffer, size_t count)
>  {
> -	Node *e;
> +	Node *e = NULL;
>  	int memsize, err;
>  	char *buf, *p;
>  	char del;
> @@ -297,6 +354,8 @@ static Node *create_entry(const char __u
>  	if (copy_from_user(buf, buffer, count))
>  		goto efault;
>  
> +	spin_lock_init(&e->auto_disable_lock);
> +
>  	del = *p++;	/* delimeter */
>  
>  	pr_debug("register: delim: %#x {%c}\n", del, del);
> @@ -454,6 +513,14 @@ static Node *create_entry(const char __u
>  
>  	/* Parse the 'flags' field. */
>  	p = check_special_flags(p, e);
> +
> +	/* Parse the 'timeout' field and init the auto-disable timer. */
> +	p = setup_auto_disable(p, buf + count, e);
> +	if (IS_ERR(p)) {
> +		err = PTR_ERR(p);
> +		goto out;
> +	}
> +
>  	if (*p == '\n')
>  		p++;
>  	if (p != buf + count)
> @@ -462,12 +529,15 @@ static Node *create_entry(const char __u
>  	return e;
>  
>  out:
> +	kfree(e);
>  	return ERR_PTR(err);
>  
>  efault:
>  	kfree(e);
>  	return ERR_PTR(-EFAULT);
>  einval:
> +	if (e)
> +		kfree(e->auto_disable);
>  	kfree(e);
>  	return ERR_PTR(-EINVAL);
>  }
> @@ -499,6 +569,21 @@ static int parse_command(const char __us
>  
>  /* generic stuff */
>  
> +static void cancel_auto_disable(Node *e)
> +{
> +	struct node_timer *auto_disable = NULL;
> +
> +	spin_lock(&e->auto_disable_lock);
> +	swap(e->auto_disable, auto_disable);
> +	spin_unlock(&e->auto_disable_lock);
> +
> +	if (auto_disable) {
> +		if (del_timer_sync(&auto_disable->timer))
> +			pr_info("%s: cancelled auto-disable\n", e->name);
> +		kfree(auto_disable);
> +	}
> +}
> +
>  static void entry_status(Node *e, char *page)
>  {
>  	char *dp = page;
> @@ -559,6 +644,8 @@ static void bm_evict_inode(struct inode
>  
>  	if (e && e->flags & MISC_FMT_OPEN_FILE)
>  		filp_close(e->interp_file, NULL);
> +	if (e)
> +		cancel_auto_disable(e);
>  
>  	clear_inode(inode);
>  	kfree(e);
> @@ -588,6 +675,8 @@ bm_entry_read(struct file *file, char __
>  	ssize_t res;
>  	char *page;
>  
> +	cancel_auto_disable(e);
> +
>  	page = (char *) __get_free_page(GFP_KERNEL);
>  	if (!page)
>  		return -ENOMEM;
> @@ -607,6 +696,8 @@ static ssize_t bm_entry_write(struct fil
>  	Node *e = file_inode(file)->i_private;
>  	int res = parse_command(buffer, count);
>  
> +	cancel_auto_disable(e);
> +
>  	switch (res) {
>  	case 1:
>  		/* Disable this handler. */
> @@ -699,6 +790,9 @@ static ssize_t bm_register_write(struct
>  	list_add(&e->list, &entries);
>  	write_unlock(&entries_lock);
>  
> +	if (e->auto_disable)
> +		add_timer(&e->auto_disable->timer);
> +
>  	err = 0;
>  out2:
>  	dput(dentry);

-- 
rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05