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@xxxxxxxxx>
> > ---
> >  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