[PATCH] rbd: fix cleanup in rbd_add()

[PATCH] rbd: fix cleanup in rbd_add()

[Alex Elder]

Bjorn Helgaas pointed out that a recent commit introduced a
use-after-free condition in an error path for rbd_add().
He correctly stated:

    I think b536f69a3a5 "rbd: set up devices only for mapped images"
    introduced a use-after-free error in rbd_add():
	...
    If rbd_dev_device_setup() returns an error, we call
    rbd_dev_image_release(), which ultimately kfrees rbd_dev.
    Then we call rbd_dev_destroy(), which references fields in
    the already-freed rbd_dev struct before kfreeing it again.

The simple fix is to return the error code after the call to
rbd_dev_image_release().

Closer examination revealed that there's no need to clean up
ceph_opts or rbd_opts in that function, so fix that too.

Update some other comments that have also become out of date.

Reported-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e8d4693..632dd35 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4699,8 +4699,10 @@ out:
 	return ret;
 }

-/* Undo whatever state changes are made by v1 or v2 image probe */
-
+/*
+ * Undo whatever state changes are made by v1 or v2 header info
+ * call.
+ */
 static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
 {
 	struct rbd_image_header	*header;
@@ -4904,9 +4906,10 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev, bool mapping)
 	int tmp;

 	/*
-	 * Get the id from the image id object.  If it's not a
-	 * format 2 image, we'll get ENOENT back, and we'll assume
-	 * it's a format 1 image.
+	 * Get the id from the image id object.  Unless there's an
+	 * error, rbd_dev->spec->image_id will be filled in with
+	 * a dynamically-allocated string, and rbd_dev->image_format
+	 * will be set to either 1 or 2.
 	 */
 	ret = rbd_dev_image_id(rbd_dev);
 	if (ret)
@@ -4994,7 +4997,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 		rc = PTR_ERR(rbdc);
 		goto err_out_args;
 	}
-	ceph_opts = NULL;	/* rbd_dev client now owns this */
+	ceph_opts = NULL;	/* rbd_get_client() consumes this */

 	/* pick the pool */
 	osdc = &rbdc->client->osdc;
@@ -5033,14 +5036,14 @@ static ssize_t rbd_add(struct bus_type *bus,
 		return count;

 	rbd_dev_image_release(rbd_dev);
+
+	return rc;
+
 err_out_rbd_dev:
 	rbd_dev_destroy(rbd_dev);
 err_out_client:
 	rbd_put_client(rbdc);
 err_out_args:
-	if (ceph_opts)
-		ceph_destroy_options(ceph_opts);
-	kfree(rbd_opts);
 	rbd_spec_put(spec);
 err_out_module:
 	module_put(THIS_MODULE);
-- 
1.7.9.5

[PATCH, v2] rbd: fix cleanup in rbd_add()

[Alex Elder]

Bjorn Helgaas pointed out that a recent commit introduced a
use-after-free condition in an error path for rbd_add().
He correctly stated:

    I think b536f69a3a5 "rbd: set up devices only for mapped images"
    introduced a use-after-free error in rbd_add():
	...
    If rbd_dev_device_setup() returns an error, we call
    rbd_dev_image_release(), which ultimately kfrees rbd_dev.
    Then we call rbd_dev_destroy(), which references fields in
    the already-freed rbd_dev struct before kfreeing it again.

The simple fix is to return the error code after the call to
rbd_dev_image_release().

Closer examination revealed that there's no need to clean up
rbd_opts in that function, so fix that too.

Update some other comments that have also become out of date.

Reported-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Alex Elder <elder@inktank.com>
---
v2: call to ceph_destroy_options() moved to its own patch

 drivers/block/rbd.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ade30dd..fdbcf04 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4700,8 +4700,10 @@ out:
 	return ret;
 }

-/* Undo whatever state changes are made by v1 or v2 image probe */
-
+/*
+ * Undo whatever state changes are made by v1 or v2 header info
+ * call.
+ */
 static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
 {
 	struct rbd_image_header	*header;
@@ -4905,9 +4907,10 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev, bool mapping)
 	int tmp;

 	/*
-	 * Get the id from the image id object.  If it's not a
-	 * format 2 image, we'll get ENOENT back, and we'll assume
-	 * it's a format 1 image.
+	 * Get the id from the image id object.  Unless there's an
+	 * error, rbd_dev->spec->image_id will be filled in with
+	 * a dynamically-allocated string, and rbd_dev->image_format
+	 * will be set to either 1 or 2.
 	 */
 	ret = rbd_dev_image_id(rbd_dev);
 	if (ret)
@@ -5033,12 +5036,14 @@ static ssize_t rbd_add(struct bus_type *bus,
 		return count;

 	rbd_dev_image_release(rbd_dev);
+
+	return rc;
+
 err_out_rbd_dev:
 	rbd_dev_destroy(rbd_dev);
 err_out_client:
 	rbd_put_client(rbdc);
 err_out_args:
-	kfree(rbd_opts);
 	rbd_spec_put(spec);
 err_out_module:
 	module_put(THIS_MODULE);
-- 
1.7.9.5

Re: [PATCH, v2] rbd: fix cleanup in rbd_add()

[Josh Durgin]

On 05/16/2013 02:29 PM, Alex Elder wrote:
> Bjorn Helgaas pointed out that a recent commit introduced a
> use-after-free condition in an error path for rbd_add().
> He correctly stated:
>
>      I think b536f69a3a5 "rbd: set up devices only for mapped images"
>      introduced a use-after-free error in rbd_add():
> 	...
>      If rbd_dev_device_setup() returns an error, we call
>      rbd_dev_image_release(), which ultimately kfrees rbd_dev.
>      Then we call rbd_dev_destroy(), which references fields in
>      the already-freed rbd_dev struct before kfreeing it again.
>
> The simple fix is to return the error code after the call to
> rbd_dev_image_release().
>
> Closer examination revealed that there's no need to clean up
> rbd_opts in that function, so fix that too.
>
> Update some other comments that have also become out of date.
>
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
> v2: call to ceph_destroy_options() moved to its own patch
>
>   drivers/block/rbd.c |   17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index ade30dd..fdbcf04 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4700,8 +4700,10 @@ out:
>   	return ret;
>   }
>
> -/* Undo whatever state changes are made by v1 or v2 image probe */
> -
> +/*
> + * Undo whatever state changes are made by v1 or v2 header info
> + * call.
> + */
>   static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
>   {
>   	struct rbd_image_header	*header;
> @@ -4905,9 +4907,10 @@ static int rbd_dev_image_probe(struct rbd_device
> *rbd_dev, bool mapping)
>   	int tmp;
>
>   	/*
> -	 * Get the id from the image id object.  If it's not a
> -	 * format 2 image, we'll get ENOENT back, and we'll assume
> -	 * it's a format 1 image.
> +	 * Get the id from the image id object.  Unless there's an
> +	 * error, rbd_dev->spec->image_id will be filled in with
> +	 * a dynamically-allocated string, and rbd_dev->image_format
> +	 * will be set to either 1 or 2.
>   	 */
>   	ret = rbd_dev_image_id(rbd_dev);
>   	if (ret)
> @@ -5033,12 +5036,14 @@ static ssize_t rbd_add(struct bus_type *bus,
>   		return count;
>
>   	rbd_dev_image_release(rbd_dev);

It looks like rbd_dev_image_release() does all the needed cleanup
except the module_put(). Just adding a module_put() here would do the
trick I think, since rbd_dev_image_release() is also used for parent
images that don't call module_get() and module_put().

With that change:

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

> +
> +	return rc;
> +
>   err_out_rbd_dev:
>   	rbd_dev_destroy(rbd_dev);
>   err_out_client:
>   	rbd_put_client(rbdc);
>   err_out_args:
> -	kfree(rbd_opts);
>   	rbd_spec_put(spec);
>   err_out_module:
>   	module_put(THIS_MODULE);

Re: [PATCH, v2] rbd: fix cleanup in rbd_add()

[Alex Elder]

On 05/16/2013 08:36 PM, Josh Durgin wrote:
> On 05/16/2013 02:29 PM, Alex Elder wrote:
>> Bjorn Helgaas pointed out that a recent commit introduced a
>> use-after-free condition in an error path for rbd_add().
>> He correctly stated:
>>
>>      I think b536f69a3a5 "rbd: set up devices only for mapped images"
>>      introduced a use-after-free error in rbd_add():
>>     ...
>>      If rbd_dev_device_setup() returns an error, we call
>>      rbd_dev_image_release(), which ultimately kfrees rbd_dev.
>>      Then we call rbd_dev_destroy(), which references fields in
>>      the already-freed rbd_dev struct before kfreeing it again.
>>
>> The simple fix is to return the error code after the call to
>> rbd_dev_image_release().
>>
>> Closer examination revealed that there's no need to clean up
>> rbd_opts in that function, so fix that too.
>>
>> Update some other comments that have also become out of date.
>>
>> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>> v2: call to ceph_destroy_options() moved to its own patch
>>
>>   drivers/block/rbd.c |   17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index ade30dd..fdbcf04 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -4700,8 +4700,10 @@ out:
>>       return ret;
>>   }
>>
>> -/* Undo whatever state changes are made by v1 or v2 image probe */
>> -
>> +/*
>> + * Undo whatever state changes are made by v1 or v2 header info
>> + * call.
>> + */
>>   static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
>>   {
>>       struct rbd_image_header    *header;
>> @@ -4905,9 +4907,10 @@ static int rbd_dev_image_probe(struct rbd_device
>> *rbd_dev, bool mapping)
>>       int tmp;
>>
>>       /*
>> -     * Get the id from the image id object.  If it's not a
>> -     * format 2 image, we'll get ENOENT back, and we'll assume
>> -     * it's a format 1 image.
>> +     * Get the id from the image id object.  Unless there's an
>> +     * error, rbd_dev->spec->image_id will be filled in with
>> +     * a dynamically-allocated string, and rbd_dev->image_format
>> +     * will be set to either 1 or 2.
>>        */
>>       ret = rbd_dev_image_id(rbd_dev);
>>       if (ret)
>> @@ -5033,12 +5036,14 @@ static ssize_t rbd_add(struct bus_type *bus,
>>           return count;
>>
>>       rbd_dev_image_release(rbd_dev);
> 
> It looks like rbd_dev_image_release() does all the needed cleanup
> except the module_put(). Just adding a module_put() here would do the
> trick I think, since rbd_dev_image_release() is also used for parent
> images that don't call module_get() and module_put().

It took me a bit to understand that what you're really saying
is that this path is missing a module_put() call...

(But that seems to be the only thing missing.)

I'll fix that.  Actually, I may just make it
goto err_out_module instead, so we get the same
debug message when there's a problem.

					-Alex

> With that change:
> 
> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 
>> +
>> +    return rc;
>> +
>>   err_out_rbd_dev:
>>       rbd_dev_destroy(rbd_dev);
>>   err_out_client:
>>       rbd_put_client(rbdc);
>>   err_out_args:
>> -    kfree(rbd_opts);
>>       rbd_spec_put(spec);
>>   err_out_module:
>>       module_put(THIS_MODULE);
>