* [PATCH] Fix NULL pointer dereference in bl_free_device(). @ 2016-07-19 11:30 Artem Savkov 2016-07-19 13:57 ` Anna Schumaker 2016-07-19 15:21 ` Anna Schumaker 0 siblings, 2 replies; 7+ messages in thread From: Artem Savkov @ 2016-07-19 11:30 UTC (permalink / raw) To: linux-nfs; +Cc: linux-kernel, trond.myklebust, hch, Artem Savkov When bl_parse_deviceid() fails in bl_alloc_deviceid_node() on blkdev_get_by_*() step we get an pnfs_block_dev struct that is uninitialized except for bdev field which is set to whatever error blkdev_get_by_*() returns. bl_free_device() then tries to call blkdev_put() if bdev is not 0 resulting in a wrong pointer dereference. Fixing this by making sure bdev is not an error code in bl_free_device(). Signed-off-by: Artem Savkov <asavkov@redhat.com> --- fs/nfs/blocklayout/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c index 118252f..7db3af0 100644 --- a/fs/nfs/blocklayout/dev.c +++ b/fs/nfs/blocklayout/dev.c @@ -33,7 +33,7 @@ bl_free_device(struct pnfs_block_dev *dev) pr_err("failed to unregister PR key.\n"); } - if (dev->bdev) + if (dev->bdev && !IS_ERR(dev->bdev)) blkdev_put(dev->bdev, FMODE_READ | FMODE_WRITE); } } -- 2.5.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix NULL pointer dereference in bl_free_device(). 2016-07-19 11:30 [PATCH] Fix NULL pointer dereference in bl_free_device() Artem Savkov @ 2016-07-19 13:57 ` Anna Schumaker 2016-07-19 15:21 ` Anna Schumaker 1 sibling, 0 replies; 7+ messages in thread From: Anna Schumaker @ 2016-07-19 13:57 UTC (permalink / raw) To: Artem Savkov, linux-nfs; +Cc: linux-kernel, trond.myklebust, hch Hi Artem, On 07/19/2016 07:30 AM, Artem Savkov wrote: > When bl_parse_deviceid() fails in bl_alloc_deviceid_node() on > blkdev_get_by_*() step we get an pnfs_block_dev struct that is > uninitialized except for bdev field which is set to whatever error > blkdev_get_by_*() returns. bl_free_device() then tries to call > blkdev_put() if bdev is not 0 resulting in a wrong pointer dereference. > > Fixing this by making sure bdev is not an error code in bl_free_device(). > > Signed-off-by: Artem Savkov <asavkov@redhat.com> > --- > fs/nfs/blocklayout/dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c > index 118252f..7db3af0 100644 > --- a/fs/nfs/blocklayout/dev.c > +++ b/fs/nfs/blocklayout/dev.c > @@ -33,7 +33,7 @@ bl_free_device(struct pnfs_block_dev *dev) > pr_err("failed to unregister PR key.\n"); > } > > - if (dev->bdev) > + if (dev->bdev && !IS_ERR(dev->bdev)) This looks pretty straightforward, but can you use IS_ERR_OR_NULL() instead? It accomplishes the same check with slightly cleaner code :) Thanks, Anna > blkdev_put(dev->bdev, FMODE_READ | FMODE_WRITE); > } > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix NULL pointer dereference in bl_free_device(). 2016-07-19 11:30 [PATCH] Fix NULL pointer dereference in bl_free_device() Artem Savkov 2016-07-19 13:57 ` Anna Schumaker @ 2016-07-19 15:21 ` Anna Schumaker 2016-07-19 15:39 ` [PATCH v2] " Artem Savkov 1 sibling, 1 reply; 7+ messages in thread From: Anna Schumaker @ 2016-07-19 15:21 UTC (permalink / raw) To: Artem Savkov, linux-nfs; +Cc: linux-kernel, trond.myklebust, hch Hi Artem, On 07/19/2016 07:30 AM, Artem Savkov wrote: > When bl_parse_deviceid() fails in bl_alloc_deviceid_node() on > blkdev_get_by_*() step we get an pnfs_block_dev struct that is > uninitialized except for bdev field which is set to whatever error > blkdev_get_by_*() returns. bl_free_device() then tries to call > blkdev_put() if bdev is not 0 resulting in a wrong pointer dereference. > > Fixing this by making sure bdev is not an error code in bl_free_device(). > > Signed-off-by: Artem Savkov <asavkov@redhat.com> > --- > fs/nfs/blocklayout/dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c > index 118252f..7db3af0 100644 > --- a/fs/nfs/blocklayout/dev.c > +++ b/fs/nfs/blocklayout/dev.c > @@ -33,7 +33,7 @@ bl_free_device(struct pnfs_block_dev *dev) > pr_err("failed to unregister PR key.\n"); > } > > - if (dev->bdev) > + if (dev->bdev && !IS_ERR(dev->bdev)) This looks pretty straightforward, but can you use IS_ERR_OR_NULL() instead? It accomplishes the same check with slightly cleaner code :) Thanks, Anna > blkdev_put(dev->bdev, FMODE_READ | FMODE_WRITE); > } > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] Fix NULL pointer dereference in bl_free_device(). 2016-07-19 15:21 ` Anna Schumaker @ 2016-07-19 15:39 ` Artem Savkov 2016-07-21 8:09 ` Christoph Hellwig 0 siblings, 1 reply; 7+ messages in thread From: Artem Savkov @ 2016-07-19 15:39 UTC (permalink / raw) To: Anna Schumaker Cc: linux-nfs, linux-kernel, trond.myklebust, hch, Artem Savkov When bl_parse_deviceid() fails in bl_alloc_deviceid_node() on blkdev_get_by_*() step we get an pnfs_block_dev struct that is uninitialized except for bdev field which is set to whatever error blkdev_get_by_*() returns. bl_free_device() then tries to call blkdev_put() if bdev is not 0 resulting in a wrong pointer dereference. Fixing this by making sure bdev is not an error code in bl_free_device(). Signed-off-by: Artem Savkov <asavkov@redhat.com> --- fs/nfs/blocklayout/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c index 118252f..57cb800 100644 --- a/fs/nfs/blocklayout/dev.c +++ b/fs/nfs/blocklayout/dev.c @@ -33,7 +33,7 @@ bl_free_device(struct pnfs_block_dev *dev) pr_err("failed to unregister PR key.\n"); } - if (dev->bdev) + if (!IS_ERR_OR_NULL(dev->bdev)) blkdev_put(dev->bdev, FMODE_READ | FMODE_WRITE); } } -- 2.5.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Fix NULL pointer dereference in bl_free_device(). 2016-07-19 15:39 ` [PATCH v2] " Artem Savkov @ 2016-07-21 8:09 ` Christoph Hellwig 2016-07-21 11:32 ` [PATCH v3] " Artem Savkov 0 siblings, 1 reply; 7+ messages in thread From: Christoph Hellwig @ 2016-07-21 8:09 UTC (permalink / raw) To: Artem Savkov Cc: Anna Schumaker, linux-nfs, linux-kernel, trond.myklebust, hch On Tue, Jul 19, 2016 at 05:39:04PM +0200, Artem Savkov wrote: > When bl_parse_deviceid() fails in bl_alloc_deviceid_node() on > blkdev_get_by_*() step we get an pnfs_block_dev struct that is > uninitialized except for bdev field which is set to whatever error > blkdev_get_by_*() returns. bl_free_device() then tries to call > blkdev_put() if bdev is not 0 resulting in a wrong pointer dereference. > > Fixing this by making sure bdev is not an error code in bl_free_device(). > > Signed-off-by: Artem Savkov <asavkov@redhat.com> I guess this is fine to be defensive, but we should probably just ensure ->bdev is NULLed on failure. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] Fix NULL pointer dereference in bl_free_device(). 2016-07-21 8:09 ` Christoph Hellwig @ 2016-07-21 11:32 ` Artem Savkov 2016-07-21 12:30 ` Benjamin Coddington 0 siblings, 1 reply; 7+ messages in thread From: Artem Savkov @ 2016-07-21 11:32 UTC (permalink / raw) To: Christoph Hellwig Cc: Anna Schumaker, linux-nfs, linux-kernel, trond.myklebust, Artem Savkov When bl_parse_deviceid() fails in bl_alloc_deviceid_node() on blkdev_get_by_*() step we get an pnfs_block_dev struct that is uninitialized except for bdev field which is set to whatever error blkdev_get_by_*() returns. bl_free_device() then tries to call blkdev_put() if bdev is not 0 resulting in a wrong pointer dereference. Fixing this by setting bdev in struct pnfs_block_dev only if we didn't get an error from blkdev_get_by_*(). Signed-off-by: Artem Savkov <asavkov@redhat.com> --- fs/nfs/blocklayout/dev.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c index 118252f..a69ef4e 100644 --- a/fs/nfs/blocklayout/dev.c +++ b/fs/nfs/blocklayout/dev.c @@ -235,18 +235,20 @@ bl_parse_simple(struct nfs_server *server, struct pnfs_block_dev *d, struct pnfs_block_volume *volumes, int idx, gfp_t gfp_mask) { struct pnfs_block_volume *v = &volumes[idx]; + struct block_device *bdev; dev_t dev; dev = bl_resolve_deviceid(server, v, gfp_mask); if (!dev) return -EIO; - d->bdev = blkdev_get_by_dev(dev, FMODE_READ | FMODE_WRITE, NULL); - if (IS_ERR(d->bdev)) { + bdev = blkdev_get_by_dev(dev, FMODE_READ | FMODE_WRITE, NULL); + if (IS_ERR(bdev)) { printk(KERN_WARNING "pNFS: failed to open device %d:%d (%ld)\n", - MAJOR(dev), MINOR(dev), PTR_ERR(d->bdev)); - return PTR_ERR(d->bdev); + MAJOR(dev), MINOR(dev), PTR_ERR(bdev)); + return PTR_ERR(bdev); } + d->bdev = bdev; d->len = i_size_read(d->bdev->bd_inode); @@ -350,17 +352,19 @@ bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d, struct pnfs_block_volume *volumes, int idx, gfp_t gfp_mask) { struct pnfs_block_volume *v = &volumes[idx]; + struct block_device *bdev; const struct pr_ops *ops; int error; if (!bl_validate_designator(v)) return -EINVAL; - d->bdev = bl_open_dm_mpath_udev_path(v); - if (IS_ERR(d->bdev)) - d->bdev = bl_open_udev_path(v); - if (IS_ERR(d->bdev)) - return PTR_ERR(d->bdev); + bdev = bl_open_dm_mpath_udev_path(v); + if (IS_ERR(bdev)) + bdev = bl_open_udev_path(v); + if (IS_ERR(bdev)) + return PTR_ERR(bdev); + d->bdev = bdev; d->len = i_size_read(d->bdev->bd_inode); d->map = bl_map_simple; -- 2.5.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] Fix NULL pointer dereference in bl_free_device(). 2016-07-21 11:32 ` [PATCH v3] " Artem Savkov @ 2016-07-21 12:30 ` Benjamin Coddington 0 siblings, 0 replies; 7+ messages in thread From: Benjamin Coddington @ 2016-07-21 12:30 UTC (permalink / raw) To: Artem Savkov Cc: Christoph Hellwig, Anna Schumaker, linux-nfs, linux-kernel, trond.myklebust On 21 Jul 2016, at 7:32, Artem Savkov wrote: > When bl_parse_deviceid() fails in bl_alloc_deviceid_node() on > blkdev_get_by_*() step we get an pnfs_block_dev struct that is > uninitialized except for bdev field which is set to whatever error > blkdev_get_by_*() returns. bl_free_device() then tries to call > blkdev_put() if bdev is not 0 resulting in a wrong pointer > dereference. > > Fixing this by setting bdev in struct pnfs_block_dev only if we didn't > get an error from blkdev_get_by_*(). > > Signed-off-by: Artem Savkov <asavkov@redhat.com> Looks good to me. Reviewed-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/blocklayout/dev.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c > index 118252f..a69ef4e 100644 > --- a/fs/nfs/blocklayout/dev.c > +++ b/fs/nfs/blocklayout/dev.c > @@ -235,18 +235,20 @@ bl_parse_simple(struct nfs_server *server, > struct pnfs_block_dev *d, > struct pnfs_block_volume *volumes, int idx, gfp_t gfp_mask) > { > struct pnfs_block_volume *v = &volumes[idx]; > + struct block_device *bdev; > dev_t dev; > > dev = bl_resolve_deviceid(server, v, gfp_mask); > if (!dev) > return -EIO; > > - d->bdev = blkdev_get_by_dev(dev, FMODE_READ | FMODE_WRITE, NULL); > - if (IS_ERR(d->bdev)) { > + bdev = blkdev_get_by_dev(dev, FMODE_READ | FMODE_WRITE, NULL); > + if (IS_ERR(bdev)) { > printk(KERN_WARNING "pNFS: failed to open device %d:%d (%ld)\n", > - MAJOR(dev), MINOR(dev), PTR_ERR(d->bdev)); > - return PTR_ERR(d->bdev); > + MAJOR(dev), MINOR(dev), PTR_ERR(bdev)); > + return PTR_ERR(bdev); > } > + d->bdev = bdev; > > > d->len = i_size_read(d->bdev->bd_inode); > @@ -350,17 +352,19 @@ bl_parse_scsi(struct nfs_server *server, struct > pnfs_block_dev *d, > struct pnfs_block_volume *volumes, int idx, gfp_t gfp_mask) > { > struct pnfs_block_volume *v = &volumes[idx]; > + struct block_device *bdev; > const struct pr_ops *ops; > int error; > > if (!bl_validate_designator(v)) > return -EINVAL; > > - d->bdev = bl_open_dm_mpath_udev_path(v); > - if (IS_ERR(d->bdev)) > - d->bdev = bl_open_udev_path(v); > - if (IS_ERR(d->bdev)) > - return PTR_ERR(d->bdev); > + bdev = bl_open_dm_mpath_udev_path(v); > + if (IS_ERR(bdev)) > + bdev = bl_open_udev_path(v); > + if (IS_ERR(bdev)) > + return PTR_ERR(bdev); > + d->bdev = bdev; > > d->len = i_size_read(d->bdev->bd_inode); > d->map = bl_map_simple; > -- > 2.5.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-21 12:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-19 11:30 [PATCH] Fix NULL pointer dereference in bl_free_device() Artem Savkov 2016-07-19 13:57 ` Anna Schumaker 2016-07-19 15:21 ` Anna Schumaker 2016-07-19 15:39 ` [PATCH v2] " Artem Savkov 2016-07-21 8:09 ` Christoph Hellwig 2016-07-21 11:32 ` [PATCH v3] " Artem Savkov 2016-07-21 12:30 ` Benjamin Coddington
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.