Messages in this thread | | | Subject | Re: [PATCH v2] tty: rocket: Fix a kernel address leak in rp_ioctl | From | Mukesh Ojha <> | Date | Sun, 31 Mar 2019 13:37:56 +0530 |
| |
On 3/31/2019 11:02 AM, 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 happens. > > This patch sets all the pointer fields to NULL before copy the > object to user space to avoid kernel address leakage.
Should not this be done like provide userspace the stuff they want?
> > Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com> > --- > drivers/tty/rocket.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/rocket.c b/drivers/tty/rocket.c > index b121d8f8f3d7..a7bcf44b61bd 100644 > --- a/drivers/tty/rocket.c > +++ b/drivers/tty/rocket.c > @@ -1271,6 +1271,34 @@ 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) Should not this void * argp be struct r_port __user ? > +{ > + struct r_port *new; > + int ret = 0; > + > + new = kzalloc(sizeof (struct r_port), GFP_KERNEL)
why there is space before sizeof everywhere ?
> ; > + if (!new) > + return -ENOMEM; > + memcpy(new, info, sizeof (struct r_port)); > + 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->xmit_buf = NULL; > + memset(&new->write_mtx.wait_list, 0, sizeof (struct list_head));
Are we sure nothing is missed ?
Are the other information use by userspace ?
> + 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 +1312,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);
| |