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@xxxxxxxxxx>
>> Signed-off-by: Alex Elder <elder@xxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxx>
>
>> +
>> + 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);
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
