| Message ID | 20230904035220.48164-1-yiyang13@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] tty: vcc: Add check for kstrdup() in vcc_probe() | expand |
On Mon, 4 Sep 2023 11:52:20 +0800 Yi Yang <yiyang13@huawei.com> wrote: > Add check for the return value of kstrdup() and return the error, if it > fails in order to avoid NULL pointer dereference. > > Fixes: 5d171050e28f ("sparc64: vcc: Enable VCC port probe and removal") > Signed-off-by: Yi Yang <yiyang13@huawei.com> > --- > V2: Add goto target for error paths. > --- > drivers/tty/vcc.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/vcc.c b/drivers/tty/vcc.c > index a39ed981bfd3..5b625f20233b 100644 > --- a/drivers/tty/vcc.c > +++ b/drivers/tty/vcc.c > @@ -579,18 +579,22 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) > return -ENOMEM; > > name = kstrdup(dev_name(&vdev->dev), GFP_KERNEL); > + if (!name) { > + rv = -ENOMEM; > + goto free_port; Hi, at this point, the port is not yet allocated, so you should not jump to free_port. You should simply return with -ENOMEM. > + } > > rv = vio_driver_init(&port->vio, vdev, VDEV_CONSOLE_CON, vcc_versions, > ARRAY_SIZE(vcc_versions), NULL, name); > if (rv) > - goto free_port; > + goto free_name; > > port->vio.debug = vcc_dbg_vio; > vcc_ldc_cfg.debug = vcc_dbg_ldc; > > rv = vio_ldc_alloc(&port->vio, &vcc_ldc_cfg, port); > if (rv) > - goto free_port; > + goto free_name; You should still jump to free_port, not free_name, after seeing my comments below > > spin_lock_init(&port->lock); > > @@ -624,6 +628,11 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) > goto unreg_tty; > } > port->domain = kstrdup(domain, GFP_KERNEL); > + if (!port->domain) { > + rv = -ENOMEM; > + goto unreg_tty; > + } > + > > mdesc_release(hp); > > @@ -653,8 +662,9 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) > vcc_table_remove(port->index); > free_ldc: > vio_ldc_free(&port->vio); > -free_port: > +free_name: > kfree(name); > +free_port: > kfree(port); free_name should come after free_port... Hugo. > > return rv; > -- > 2.17.1 >
On 2023/9/5 22:19, Hugo Villeneuve wrote: > On Mon, 4 Sep 2023 11:52:20 +0800 > Yi Yang <yiyang13@huawei.com> wrote: > >> Add check for the return value of kstrdup() and return the error, if it >> fails in order to avoid NULL pointer dereference. >> >> Fixes: 5d171050e28f ("sparc64: vcc: Enable VCC port probe and removal") >> Signed-off-by: Yi Yang <yiyang13@huawei.com> >> --- >> V2: Add goto target for error paths. >> --- >> drivers/tty/vcc.c | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/vcc.c b/drivers/tty/vcc.c >> index a39ed981bfd3..5b625f20233b 100644 >> --- a/drivers/tty/vcc.c >> +++ b/drivers/tty/vcc.c >> @@ -579,18 +579,22 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) >> return -ENOMEM; >> >> name = kstrdup(dev_name(&vdev->dev), GFP_KERNEL); >> + if (!name) { >> + rv = -ENOMEM; >> + goto free_port; > > Hi, > at this point, the port is not yet allocated, so you should not jump to > free_port. You should simply return with -ENOMEM. > The port was already allocated by kzalloc(), and should be free before return -ENOMEM. > >> + } >> >> rv = vio_driver_init(&port->vio, vdev, VDEV_CONSOLE_CON, vcc_versions, >> ARRAY_SIZE(vcc_versions), NULL, name); >> if (rv) >> - goto free_port; >> + goto free_name; >> >> port->vio.debug = vcc_dbg_vio; >> vcc_ldc_cfg.debug = vcc_dbg_ldc; >> >> rv = vio_ldc_alloc(&port->vio, &vcc_ldc_cfg, port); >> if (rv) >> - goto free_port; >> + goto free_name; > > You should still jump to free_port, not free_name, after seeing my > comments below > > >> >> spin_lock_init(&port->lock); >> >> @@ -624,6 +628,11 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) >> goto unreg_tty; >> } >> port->domain = kstrdup(domain, GFP_KERNEL); >> + if (!port->domain) { >> + rv = -ENOMEM; >> + goto unreg_tty; >> + } >> + >> and should be free before return -ENOMEM. >> mdesc_release(hp); >> >> @@ -653,8 +662,9 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) >> vcc_table_remove(port->index); >> free_ldc: >> vio_ldc_free(&port->vio); >> -free_port: >> +free_name: >> kfree(name); >> +free_port: >> kfree(port); > > free_name should come after free_port... > > Hugo. The release process should be in reverse order. -- Yi Yang > > >> >> return rv; >> -- >> 2.17.1 >> > > . >
On Thu, 7 Sep 2023 09:25:12 +0800 "yiyang (D)" <yiyang13@huawei.com> wrote: > On 2023/9/5 22:19, Hugo Villeneuve wrote: > > On Mon, 4 Sep 2023 11:52:20 +0800 > > Yi Yang <yiyang13@huawei.com> wrote: > > > >> Add check for the return value of kstrdup() and return the error, if it > >> fails in order to avoid NULL pointer dereference. > >> > >> Fixes: 5d171050e28f ("sparc64: vcc: Enable VCC port probe and removal") > >> Signed-off-by: Yi Yang <yiyang13@huawei.com> > >> --- > >> V2: Add goto target for error paths. > >> --- > >> drivers/tty/vcc.c | 16 +++++++++++++--- > >> 1 file changed, 13 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/tty/vcc.c b/drivers/tty/vcc.c > >> index a39ed981bfd3..5b625f20233b 100644 > >> --- a/drivers/tty/vcc.c > >> +++ b/drivers/tty/vcc.c > >> @@ -579,18 +579,22 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) > >> return -ENOMEM; > >> > >> name = kstrdup(dev_name(&vdev->dev), GFP_KERNEL); > >> + if (!name) { > >> + rv = -ENOMEM; > >> + goto free_port; > > > > Hi, > > at this point, the port is not yet allocated, so you should not jump to > > free_port. You should simply return with -ENOMEM. > > > The port was already allocated by kzalloc(), and should be free before > return -ENOMEM. You are right, dismiss all my comments. Hugo. > > > >> + } > >> > >> rv = vio_driver_init(&port->vio, vdev, VDEV_CONSOLE_CON, vcc_versions, > >> ARRAY_SIZE(vcc_versions), NULL, name); > >> if (rv) > >> - goto free_port; > >> + goto free_name; > >> > >> port->vio.debug = vcc_dbg_vio; > >> vcc_ldc_cfg.debug = vcc_dbg_ldc; > >> > >> rv = vio_ldc_alloc(&port->vio, &vcc_ldc_cfg, port); > >> if (rv) > >> - goto free_port; > >> + goto free_name; > > > > You should still jump to free_port, not free_name, after seeing my > > comments below > > > > > >> > >> spin_lock_init(&port->lock); > >> > >> @@ -624,6 +628,11 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) > >> goto unreg_tty; > >> } > >> port->domain = kstrdup(domain, GFP_KERNEL); > >> + if (!port->domain) { > >> + rv = -ENOMEM; > >> + goto unreg_tty; > >> + } > >> + > >> and should be free before return -ENOMEM. > >> mdesc_release(hp); > >> > >> @@ -653,8 +662,9 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) > >> vcc_table_remove(port->index); > >> free_ldc: > >> vio_ldc_free(&port->vio); > >> -free_port: > >> +free_name: > >> kfree(name); > >> +free_port: > >> kfree(port); > > > > free_name should come after free_port... > > > > Hugo. > The release process should be in reverse order. > > -- > Yi Yang > > > > > >> > >> return rv; > >> -- > >> 2.17.1 > >> > > > > . > > >
On 04. 09. 23, 5:52, Yi Yang wrote: > Add check for the return value of kstrdup() and return the error, if it > fails in order to avoid NULL pointer dereference. > > Fixes: 5d171050e28f ("sparc64: vcc: Enable VCC port probe and removal") > Signed-off-by: Yi Yang <yiyang13@huawei.com> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
diff --git a/drivers/tty/vcc.c b/drivers/tty/vcc.c index a39ed981bfd3..5b625f20233b 100644 --- a/drivers/tty/vcc.c +++ b/drivers/tty/vcc.c @@ -579,18 +579,22 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) return -ENOMEM; name = kstrdup(dev_name(&vdev->dev), GFP_KERNEL); + if (!name) { + rv = -ENOMEM; + goto free_port; + } rv = vio_driver_init(&port->vio, vdev, VDEV_CONSOLE_CON, vcc_versions, ARRAY_SIZE(vcc_versions), NULL, name); if (rv) - goto free_port; + goto free_name; port->vio.debug = vcc_dbg_vio; vcc_ldc_cfg.debug = vcc_dbg_ldc; rv = vio_ldc_alloc(&port->vio, &vcc_ldc_cfg, port); if (rv) - goto free_port; + goto free_name; spin_lock_init(&port->lock); @@ -624,6 +628,11 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) goto unreg_tty; } port->domain = kstrdup(domain, GFP_KERNEL); + if (!port->domain) { + rv = -ENOMEM; + goto unreg_tty; + } + mdesc_release(hp); @@ -653,8 +662,9 @@ static int vcc_probe(struct vio_dev *vdev, const struct vio_device_id *id) vcc_table_remove(port->index); free_ldc: vio_ldc_free(&port->vio); -free_port: +free_name: kfree(name); +free_port: kfree(port); return rv;
Add check for the return value of kstrdup() and return the error, if it fails in order to avoid NULL pointer dereference. Fixes: 5d171050e28f ("sparc64: vcc: Enable VCC port probe and removal") Signed-off-by: Yi Yang <yiyang13@huawei.com> --- V2: Add goto target for error paths. --- drivers/tty/vcc.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)