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 <[email protected]>
> ---
> 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
