[PATCH v3] tty: rocket: Fix a kernel address leakage in rp_ioctl

[PATCH v3] tty: rocket: Fix a kernel address leakage in rp_ioctl

[Fuqian Huang]

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@gmail.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;
+}
+
 /*  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);
-- 
2.11.0

Re: [PATCH v3] tty: rocket: Fix a kernel address leakage in rp_ioctl

[Jiri Slaby]

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@gmail.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

Re: [PATCH v3] tty: rocket: Fix a kernel address leakage in rp_ioctl

[Greg Kroah-Hartman]

On Tue, Apr 02, 2019 at 10:34:08AM +0200, Jiri Slaby wrote:
> 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@gmail.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.

I agree, let's just delete the ioctl completly.

Fuqian, can you send a patch doing that please?

thanks,

greg k-h