Re: [bug report] block/rnbd-clt: Dynamically alloc buffer for pathname & blk_symlink_name

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Dec 08, 2020 at 02:24:40PM +0100, Jinpu Wang wrote:
> Hi,
> 
> On Tue, Dec 8, 2020 at 2:21 PM Haris Iqbal <haris.iqbal@cloud.ionos.com> wrote:
> >
> > On Tue, Dec 8, 2020 at 5:45 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > Hello Md Haris Iqbal,
> > >
> > > This is a semi-automatic email about new static checker warnings.
> > >
> > > The patch 64e8a6ece1a5: "block/rnbd-clt: Dynamically alloc buffer for
> > > pathname & blk_symlink_name" from Nov 26, 2020, leads to the
> > > following Smatch complaint:
> > >
> > >     drivers/block/rnbd/rnbd-clt-sysfs.c:525 rnbd_clt_add_dev_symlink()
> > >     error: uninitialized symbol 'ret'.
> > >
> > >     drivers/block/rnbd/rnbd-clt-sysfs.c:524 rnbd_clt_add_dev_symlink()
> > >     error: we previously assumed 'dev->blk_symlink_name' could be null (see line 500)
> > >
> > > drivers/block/rnbd/rnbd-clt-sysfs.c
> > >    493  static int rnbd_clt_add_dev_symlink(struct rnbd_clt_dev *dev)
> > >    494  {
> > >    495          struct kobject *gd_kobj = &disk_to_dev(dev->gd)->kobj;
> > >    496          int ret, len;
> > >    497
> > >    498          len = strlen(dev->pathname) + strlen(dev->sess->sessname) + 2;
> > >    499          dev->blk_symlink_name = kzalloc(len, GFP_KERNEL);
> > >    500          if (!dev->blk_symlink_name) {
> > >    501                  rnbd_clt_err(dev, "Failed to allocate memory for blk_symlink_name\n");
> > >    502                  goto out_err;
> > >
> > > ret = -ENOMEM; here
> > >
> > >    503          }
> > >    504
> > >    505          ret = rnbd_clt_get_path_name(dev, dev->blk_symlink_name,
> > >    506                                        len);
> > >    507          if (ret) {
> > >    508                  rnbd_clt_err(dev, "Failed to get /sys/block symlink path, err: %d\n",
> > >    509                                ret);
> > >    510                  goto out_err;
> > >    511          }
> > >    512
> > >    513          ret = sysfs_create_link(rnbd_devs_kobj, gd_kobj,
> > >    514                                  dev->blk_symlink_name);
> > >    515          if (ret) {
> > >    516                  rnbd_clt_err(dev, "Creating /sys/block symlink failed, err: %d\n",
> > >    517                                ret);
> > >    518                  goto out_err;
> > >    519          }
> > >    520
> > >    521          return 0;
> > >    522
> > >    523  out_err:
> > >    524          dev->blk_symlink_name[0] = '\0';
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^^
> > > This will oops if the kzalloc() fails.
> >
> > Thanks. Will send a patch soon.
> already fixed in block tree by Colin
> >https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-5.11/drivers&id=733c15bd3a944b8eeaacdddf061759b6a83dd3f4 

It's a weird thing that we don't free dev->blk_symlink_name if there
is an error.  It's impossible for rnbd_clt_get_path_name() to actually
return an error in the current code, but if it did then only the last
character of dev->blk_symlink_name[] is initialized.

regards,
dan carpenter