LinuxLists.cc - [PATCH] Fix NULL pointer dereference in bl_free

2016-07-19 11:31:18

Subject: [PATCH] Fix NULL pointer dereference in bl_free_device().

2016-07-19 15:21:13

Subject: Re: [PATCH] Fix NULL pointer dereference in bl_free_device().

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 <[email protected]>
> ---
> 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);
> }
> }
>

2016-07-19 15:21:15

by Anna Schumaker

[permalink] [raw]

Subject: Re: [PATCH] Fix NULL pointer dereference in bl_free_device().

2016-07-19 15:39:55

by Artem Savkov

[permalink] [raw]

Subject: [PATCH v2] Fix NULL pointer dereference in bl_free_device().

2016-07-21 08:09:13

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH v2] Fix NULL pointer dereference in bl_free_device().

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 <[email protected]>

I guess this is fine to be defensive, but we should probably just ensure
->bdev is NULLed on failure.

2016-07-21 11:32:42

by Artem Savkov

[permalink] [raw]

Subject: [PATCH v3] Fix NULL pointer dereference in bl_free_device().

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 <[email protected]>
---
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

2016-07-21 12:29:15

by Benjamin Coddington

[permalink] [raw]

Subject: Re: [PATCH v3] Fix NULL pointer dereference in bl_free_device().

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 <[email protected]>

Looks good to me.

Reviewed-by: Benjamin Coddington <[email protected]>

> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html