linux-kernel - Re: [PATCH v3] tty: rocket: Fix a kernel address leakage in rp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e8e20b1b-d853-968e-d380-a142877e9b9d@suse.cz>
Date:   Tue, 2 Apr 2019 10:34:08 +0200
From:   Jiri Slaby <jslaby@...e.cz>
To:     Fuqian Huang <huangfq.daxian@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] tty: rocket: Fix a kernel address leakage in rp_ioctl

On 31. 03. 19, 13:51, Fuqian Huang wrote:
> If the cmd is RCPK_GET_STRUCT, copy_to_user will copy info to
> user space. As info->port.ops is the address of a constant object
> rocket_port_ops (assigned in init_r_port), a kernel address leakage.
> 
> This patch sets all the pointer fields to NULL before copy the
> object to user space to avoid kernel address leakage.
> 
> All pointer fields except
> .magic field and the pointer in .dep_map field in struct mutex
> which only exist under debug configurations of CONFIG_DEBUG_MUTEXES
> and CONFIG_DEBUG_LOCK_ALLOC
> are set to NULL.
> 
> The set NULL pointer fields are address of kernel objects:
>  - pointers to constant objects: port.ops, port.client_ops
>  - pointers to tty_struct instance
>  - pointer.xmit_buf (allocated in rp_open)
>  - pointer fields of wait_queue_head_t and struct mutex
> 
> I cannot think of a scenario where user space needs the address of
> these kernel objects. So I set them all NULL before copy_to_user.
> Another reason is that I have checked all uses of copy_to_user and
> copy_from_user in this file, they do not use any of these pointer
> fields. So these pointer fields can be set NULL safely.
> 
> Signed-off-by: Fuqian Huang <huangfq.daxian@...il.com>
> ---
>  drivers/tty/rocket.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/rocket.c b/drivers/tty/rocket.c
> index b121d8f8f3d7..df0b8ebab266 100644
> --- a/drivers/tty/rocket.c
> +++ b/drivers/tty/rocket.c
> @@ -1271,6 +1271,48 @@ static int get_version(struct r_port *info, struct rocket_version __user *retver
>  	return 0;
>  }
>  
> +static int get_struct(struct r_port *info, void *argp)
> +{
> +	struct r_port *new;
> +	int ret = 0;
> +
> +	new = kzalloc(sizeof(struct r_port), GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +	memcpy(new, info, sizeof(struct r_port));
> +	new->port.buf.head = NULL;
> +	memset(&new->port.buf.work.entry, 0, sizeof(struct list_head));
> +	new->port.buf.work.func = NULL;
> +#ifdef CONFIG_LOCKDEP
> +	new->port.buf.work.lockdep_map.key = NULL;
> +	memset(&new->port.buf.work.lockdep_map.class_cache, 0,
> +		sizeof(struct lock_class *) * NR_LOCKDEP_CACHING_CLASSES);
> +	new->port.buf.work.lockdep_map.name = NULL;
> +#endif
> +	memset(&new->port.buf.lock.wait_list, 0, sizeof(struct list_head));
> +	new->port.buf.sentinel.next = NULL;
> +	memset(&new->port.buf.free, 0, sizeof(struct llist_head));
> +	new->port.buf.tail = NULL;
> +	new->port.tty = NULL;
> +	new->port.itty = NULL;
> +	new->port.ops = NULL;
> +	new->port.client_ops = NULL;
> +	memset(&new->port.open_wait.head, 0, sizeof(struct list_head));
> +	memset(&new->port.delta_msr_wait.head, 0, sizeof(struct list_head));
> +	memset(&new->port.mutex.wait_list, 0, sizeof(struct list_head));
> +	memset(&new->port.buf_mutex.wait_list, 0, sizeof(struct list_head));
> +	new->port.xmit_buf = NULL;
> +	new->port.client_data = NULL;
> +	new->ctlp = NULL;
> +	new->channel.CtlP = NULL;
> +	new->xmit_buf = NULL;
> +	memset(&new->write_mtx.wait_list, 0, sizeof(struct list_head));
> +	if (copy_to_user(argp, new, sizeof(struct r_port)))
> +		ret = -EFAULT;
> +	kfree(new);
> +	return ret;

Ugh, no.

1) The structure is defined as rocket _internal-only_.
2) It changed many times over time (whenever tty_port was changed at least).
3) Differs depending on various CONFIG_* options.

I seriously doubt anybody used the ioctl, ever. (What size would they
pass, so that copy_to_user won't fail?)

So now, I am in favor of killing the ioctl completely. We also never
exposed the ioctl number to userspace.

> +}
> +
>  /*  IOCTL call handler into the driver */
>  static int rp_ioctl(struct tty_struct *tty,
>  		    unsigned int cmd, unsigned long arg)
> @@ -1284,8 +1326,7 @@ static int rp_ioctl(struct tty_struct *tty,
>  
>  	switch (cmd) {
>  	case RCKP_GET_STRUCT:
> -		if (copy_to_user(argp, info, sizeof (struct r_port)))
> -			ret = -EFAULT;
> +		ret = get_struct(info, argp);
>  		break;
>  	case RCKP_GET_CONFIG:
>  		ret = get_config(info, argp);
> 

thanks,
-- 
js
suse labs