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