* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).