[dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Luo Meng]

From: Luo Meng <luomeng12@huawei.com>

If dm_get_device() create dd in multipath_message(),
and then call table_deps() after dm_put_table_device(),
it will lead to concurrency UAF bugs.

One of the concurrency UAF can be shown as below:

         (USE)                        |    (FREE)
                                      |  target_message
                                      |    multipath_message
                                      |      dm_put_device
                                      |        dm_put_table_device #
                                      |          kfree(td)  # table_device *td
ioctl # DM_TABLE_DEPS_CMD             |         ...
  table_deps                          |         ...
  dm_get_live_or_inactive_table       |         ...
    retrieve_dep                      |         ...
    list_for_each_entry               |         ...
      deps->dev[count++] =            |         ...
          huge_encode_dev             |         ...
          (dd->dm_dev->bdev->bd_dev)  |        list_del(&dd->list)
                                      |        kfree(dd) # dm_dev_internal

The root cause of UAF bugs is that find_device() failed in
dm_get_device() and will create dd and refcount set 1, kfree()
in dm_put_table() is not protected. When td, which there are
still pointers point to, is released, the concurrency UAF bug
will happen.

This patch add a flag to determine whether to create a new dd.

Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
Signed-off-by: Luo Meng <luomeng12@huawei.com>
---
 drivers/md/dm-mpath.c         |  2 +-
 drivers/md/dm-table.c         | 43 +++++++++++++++++++++--------------
 include/linux/device-mapper.h |  2 ++
 3 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 0e325469a252..1f51059520ac 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
 		goto out;
 	}
 
-	r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
+	r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
 	if (r) {
 		DMWARN("message: error getting device %s",
 		       argv[1]);
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index d8034ff0cb24..ad18a41f1608 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
 }
 EXPORT_SYMBOL_GPL(dm_get_dev_t);
 
-/*
- * Add a device to the list, or just increment the usage count if
- * it's already present.
- */
-int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
-		  struct dm_dev **result)
+int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
+		    struct dm_dev **result, bool create_dd)
 {
 	int r;
 	dev_t dev;
@@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
 
 	dd = find_device(&t->devices, dev);
 	if (!dd) {
-		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
-		if (!dd)
-			return -ENOMEM;
-
-		if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
-			kfree(dd);
-			return r;
-		}
+		if (create_dd) {
+			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
+			if (!dd)
+				return -ENOMEM;
 
-		refcount_set(&dd->count, 1);
-		list_add(&dd->list, &t->devices);
-		goto out;
+			if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
+				kfree(dd);
+				return r;
+			}
 
+			refcount_set(&dd->count, 1);
+			list_add(&dd->list, &t->devices);
+			goto out;
+		} else
+			return -ENODEV;
 	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
 		r = upgrade_mode(dd, mode, t->md);
 		if (r)
@@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
 	*result = dd->dm_dev;
 	return 0;
 }
+EXPORT_SYMBOL(__dm_get_device);
+
+/*
+ * Add a device to the list, or just increment the usage count if
+ * it's already present.
+ */
+int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
+		  struct dm_dev **result)
+{
+	return __dm_get_device(ti, path, mode, result, true);
+}
 EXPORT_SYMBOL(dm_get_device);
 
 static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 04c6acf7faaa..ef4031a844b6 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
 int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
 		  struct dm_dev **result);
 void dm_put_device(struct dm_target *ti, struct dm_dev *d);
+int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
+		    struct dm_dev **result, bool create_dd);
 
 /*
  * Information about a target type
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Mike Snitzer]

On Mon, Oct 10 2022 at 10:41P -0400,
Luo Meng <luomeng@huaweicloud.com> wrote:

> From: Luo Meng <luomeng12@huawei.com>
> 
> If dm_get_device() create dd in multipath_message(),
> and then call table_deps() after dm_put_table_device(),
> it will lead to concurrency UAF bugs.
> 
> One of the concurrency UAF can be shown as below:
> 
>          (USE)                        |    (FREE)
>                                       |  target_message
>                                       |    multipath_message
>                                       |      dm_put_device
>                                       |        dm_put_table_device #
>                                       |          kfree(td)  # table_device *td
> ioctl # DM_TABLE_DEPS_CMD             |         ...
>   table_deps                          |         ...
>   dm_get_live_or_inactive_table       |         ...
>     retrieve_dep                      |         ...
>     list_for_each_entry               |         ...
>       deps->dev[count++] =            |         ...
>           huge_encode_dev             |         ...
>           (dd->dm_dev->bdev->bd_dev)  |        list_del(&dd->list)
>                                       |        kfree(dd) # dm_dev_internal
> 
> The root cause of UAF bugs is that find_device() failed in
> dm_get_device() and will create dd and refcount set 1, kfree()
> in dm_put_table() is not protected. When td, which there are
> still pointers point to, is released, the concurrency UAF bug
> will happen.
> 
> This patch add a flag to determine whether to create a new dd.
> 
> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
> Signed-off-by: Luo Meng <luomeng12@huawei.com>
> ---
>  drivers/md/dm-mpath.c         |  2 +-
>  drivers/md/dm-table.c         | 43 +++++++++++++++++++++--------------
>  include/linux/device-mapper.h |  2 ++
>  3 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 0e325469a252..1f51059520ac 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
>  		goto out;
>  	}
>  
> -	r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
> +	r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
>  	if (r) {
>  		DMWARN("message: error getting device %s",
>  		       argv[1]);
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index d8034ff0cb24..ad18a41f1608 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
>  }
>  EXPORT_SYMBOL_GPL(dm_get_dev_t);
>  
> -/*
> - * Add a device to the list, or just increment the usage count if
> - * it's already present.
> - */
> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> -		  struct dm_dev **result)
> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> +		    struct dm_dev **result, bool create_dd)
>  {
>  	int r;
>  	dev_t dev;
> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>  
>  	dd = find_device(&t->devices, dev);
>  	if (!dd) {
> -		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> -		if (!dd)
> -			return -ENOMEM;
> -
> -		if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> -			kfree(dd);
> -			return r;
> -		}
> +		if (create_dd) {
> +			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> +			if (!dd)
> +				return -ENOMEM;
>  
> -		refcount_set(&dd->count, 1);
> -		list_add(&dd->list, &t->devices);
> -		goto out;
> +			if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> +				kfree(dd);
> +				return r;
> +			}
>  
> +			refcount_set(&dd->count, 1);
> +			list_add(&dd->list, &t->devices);
> +			goto out;
> +		} else
> +			return -ENODEV;
>  	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>  		r = upgrade_mode(dd, mode, t->md);
>  		if (r)
> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>  	*result = dd->dm_dev;
>  	return 0;
>  }
> +EXPORT_SYMBOL(__dm_get_device);
> +
> +/*
> + * Add a device to the list, or just increment the usage count if
> + * it's already present.
> + */
> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> +		  struct dm_dev **result)
> +{
> +	return __dm_get_device(ti, path, mode, result, true);
> +}
>  EXPORT_SYMBOL(dm_get_device);
>  
>  static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
> index 04c6acf7faaa..ef4031a844b6 100644
> --- a/include/linux/device-mapper.h
> +++ b/include/linux/device-mapper.h
> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
>  int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>  		  struct dm_dev **result);
>  void dm_put_device(struct dm_target *ti, struct dm_dev *d);
> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> +		    struct dm_dev **result, bool create_dd);
>  
>  /*
>   * Information about a target type
> -- 
> 2.31.1
> 

This patch is treating the one multipath case like it is only consumer
of dm_get_device() that might have this issue. It feels too focused on
an instance where dm_get_device()'s side-effect of creating the device
is unwelcome. Feels like you're treating the symptom rather than the
problem (e.g. that the "kfree() in dm_put_table() is not protected"?)

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Luo Meng]

在 2022/10/18 3:56, Mike Snitzer 写道:
> On Mon, Oct 10 2022 at 10:41P -0400,
> Luo Meng <luomeng@huaweicloud.com> wrote:
> 
>> From: Luo Meng <luomeng12@huawei.com>
>>
>> If dm_get_device() create dd in multipath_message(),
>> and then call table_deps() after dm_put_table_device(),
>> it will lead to concurrency UAF bugs.
>>
>> One of the concurrency UAF can be shown as below:
>>
>>           (USE)                        |    (FREE)
>>                                        |  target_message
>>                                        |    multipath_message
>>                                        |      dm_put_device
>>                                        |        dm_put_table_device #
>>                                        |          kfree(td)  # table_device *td
>> ioctl # DM_TABLE_DEPS_CMD             |         ...
>>    table_deps                          |         ...
>>    dm_get_live_or_inactive_table       |         ...
>>      retrieve_dep                      |         ...
>>      list_for_each_entry               |         ...
>>        deps->dev[count++] =            |         ...
>>            huge_encode_dev             |         ...
>>            (dd->dm_dev->bdev->bd_dev)  |        list_del(&dd->list)
>>                                        |        kfree(dd) # dm_dev_internal
>>
>> The root cause of UAF bugs is that find_device() failed in
>> dm_get_device() and will create dd and refcount set 1, kfree()
>> in dm_put_table() is not protected. When td, which there are
>> still pointers point to, is released, the concurrency UAF bug
>> will happen.
>>
>> This patch add a flag to determine whether to create a new dd.
>>
>> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
>> Signed-off-by: Luo Meng <luomeng12@huawei.com>
>> ---
>>   drivers/md/dm-mpath.c         |  2 +-
>>   drivers/md/dm-table.c         | 43 +++++++++++++++++++++--------------
>>   include/linux/device-mapper.h |  2 ++
>>   3 files changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index 0e325469a252..1f51059520ac 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
>>   		goto out;
>>   	}
>>   
>> -	r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
>> +	r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
>>   	if (r) {
>>   		DMWARN("message: error getting device %s",
>>   		       argv[1]);
>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>> index d8034ff0cb24..ad18a41f1608 100644
>> --- a/drivers/md/dm-table.c
>> +++ b/drivers/md/dm-table.c
>> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
>>   }
>>   EXPORT_SYMBOL_GPL(dm_get_dev_t);
>>   
>> -/*
>> - * Add a device to the list, or just increment the usage count if
>> - * it's already present.
>> - */
>> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> -		  struct dm_dev **result)
>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> +		    struct dm_dev **result, bool create_dd)
>>   {
>>   	int r;
>>   	dev_t dev;
>> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>   
>>   	dd = find_device(&t->devices, dev);
>>   	if (!dd) {
>> -		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>> -		if (!dd)
>> -			return -ENOMEM;
>> -
>> -		if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
>> -			kfree(dd);
>> -			return r;
>> -		}
>> +		if (create_dd) {
>> +			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>> +			if (!dd)
>> +				return -ENOMEM;
>>   
>> -		refcount_set(&dd->count, 1);
>> -		list_add(&dd->list, &t->devices);
>> -		goto out;
>> +			if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
>> +				kfree(dd);
>> +				return r;
>> +			}
>>   
>> +			refcount_set(&dd->count, 1);
>> +			list_add(&dd->list, &t->devices);
>> +			goto out;
>> +		} else
>> +			return -ENODEV;
>>   	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>>   		r = upgrade_mode(dd, mode, t->md);
>>   		if (r)
>> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>   	*result = dd->dm_dev;
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(__dm_get_device);
>> +
>> +/*
>> + * Add a device to the list, or just increment the usage count if
>> + * it's already present.
>> + */
>> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> +		  struct dm_dev **result)
>> +{
>> +	return __dm_get_device(ti, path, mode, result, true);
>> +}
>>   EXPORT_SYMBOL(dm_get_device);
>>   
>>   static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
>> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
>> index 04c6acf7faaa..ef4031a844b6 100644
>> --- a/include/linux/device-mapper.h
>> +++ b/include/linux/device-mapper.h
>> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
>>   int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>   		  struct dm_dev **result);
>>   void dm_put_device(struct dm_target *ti, struct dm_dev *d);
>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> +		    struct dm_dev **result, bool create_dd);
>>   
>>   /*
>>    * Information about a target type
>> -- 
>> 2.31.1
>>
> 
> This patch is treating the one multipath case like it is only consumer
> of dm_get_device() that might have this issue. It feels too focused on
> an instance where dm_get_device()'s side-effect of creating the device
> is unwelcome. Feels like you're treating the symptom rather than the
> problem (e.g. that the "kfree() in dm_put_table() is not protected"?)
> 
> Mike
> 
Thanks for your reply. I know it is not the best way to slove the 
problem, however I have no idea about it.  Do you have a better way to 
fix it?

Luo Meng

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[lilingfeng (A)]

在 2022/10/18 3:56, Mike Snitzer 写道:
> On Mon, Oct 10 2022 at 10:41P -0400,
> Luo Meng <luomeng@huaweicloud.com> wrote:
>
>> From: Luo Meng <luomeng12@huawei.com>
>>
>> If dm_get_device() create dd in multipath_message(),
>> and then call table_deps() after dm_put_table_device(),
>> it will lead to concurrency UAF bugs.
>>
>> One of the concurrency UAF can be shown as below:
>>
>>           (USE)                        |    (FREE)
>>                                        |  target_message
>>                                        |    multipath_message
>>                                        |      dm_put_device
>>                                        |        dm_put_table_device #
>>                                        |          kfree(td)  # table_device *td
>> ioctl # DM_TABLE_DEPS_CMD             |         ...
>>    table_deps                          |         ...
>>    dm_get_live_or_inactive_table       |         ...
>>      retrieve_dep                      |         ...
>>      list_for_each_entry               |         ...
>>        deps->dev[count++] =            |         ...
>>            huge_encode_dev             |         ...
>>            (dd->dm_dev->bdev->bd_dev)  |        list_del(&dd->list)
>>                                        |        kfree(dd) # dm_dev_internal
>>
>> The root cause of UAF bugs is that find_device() failed in
>> dm_get_device() and will create dd and refcount set 1, kfree()
>> in dm_put_table() is not protected. When td, which there are
>> still pointers point to, is released, the concurrency UAF bug
>> will happen.
>>
>> This patch add a flag to determine whether to create a new dd.
>>
>> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
>> Signed-off-by: Luo Meng <luomeng12@huawei.com>
>> ---
>>   drivers/md/dm-mpath.c         |  2 +-
>>   drivers/md/dm-table.c         | 43 +++++++++++++++++++++--------------
>>   include/linux/device-mapper.h |  2 ++
>>   3 files changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index 0e325469a252..1f51059520ac 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
>>   		goto out;
>>   	}
>>   
>> -	r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
>> +	r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
>>   	if (r) {
>>   		DMWARN("message: error getting device %s",
>>   		       argv[1]);
>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>> index d8034ff0cb24..ad18a41f1608 100644
>> --- a/drivers/md/dm-table.c
>> +++ b/drivers/md/dm-table.c
>> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
>>   }
>>   EXPORT_SYMBOL_GPL(dm_get_dev_t);
>>   
>> -/*
>> - * Add a device to the list, or just increment the usage count if
>> - * it's already present.
>> - */
>> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> -		  struct dm_dev **result)
>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> +		    struct dm_dev **result, bool create_dd)
>>   {
>>   	int r;
>>   	dev_t dev;
>> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>   
>>   	dd = find_device(&t->devices, dev);
>>   	if (!dd) {
>> -		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>> -		if (!dd)
>> -			return -ENOMEM;
>> -
>> -		if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
>> -			kfree(dd);
>> -			return r;
>> -		}
>> +		if (create_dd) {
>> +			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>> +			if (!dd)
>> +				return -ENOMEM;
>>   
>> -		refcount_set(&dd->count, 1);
>> -		list_add(&dd->list, &t->devices);
>> -		goto out;
>> +			if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
>> +				kfree(dd);
>> +				return r;
>> +			}
>>   
>> +			refcount_set(&dd->count, 1);
>> +			list_add(&dd->list, &t->devices);
>> +			goto out;
>> +		} else
>> +			return -ENODEV;
>>   	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>>   		r = upgrade_mode(dd, mode, t->md);
>>   		if (r)
>> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>   	*result = dd->dm_dev;
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(__dm_get_device);
>> +
>> +/*
>> + * Add a device to the list, or just increment the usage count if
>> + * it's already present.
>> + */
>> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> +		  struct dm_dev **result)
>> +{
>> +	return __dm_get_device(ti, path, mode, result, true);
>> +}
>>   EXPORT_SYMBOL(dm_get_device);
>>   
>>   static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
>> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
>> index 04c6acf7faaa..ef4031a844b6 100644
>> --- a/include/linux/device-mapper.h
>> +++ b/include/linux/device-mapper.h
>> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
>>   int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>   		  struct dm_dev **result);
>>   void dm_put_device(struct dm_target *ti, struct dm_dev *d);
>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>> +		    struct dm_dev **result, bool create_dd);
>>   
>>   /*
>>    * Information about a target type
>> -- 
>> 2.31.1
>>
> This patch is treating the one multipath case like it is only consumer
> of dm_get_device() that might have this issue. It feels too focused on
> an instance where dm_get_device()'s side-effect of creating the device
> is unwelcome. Feels like you're treating the symptom rather than the
> problem (e.g. that the "kfree() in dm_put_table() is not protected"?)
>
> Mike

In other cases, kfree() in dm_put_table() is protected by srcu.
For example:
           USE                              FREE
table_deps                            dev_remove
  dm_get_live_or_inactive_table         dm_sync_table
   // lock
   srcu_read_lock(&md->io_barrier)
                                         // wait for unlock
                                         synchronize_srcu(&md->io_barrier)
   retrieve_deps
  dm_put_live_table
   // unlock
   srcu_read_unlock(&md->io_barrier...)
                                        dm_table_destroy
                                         linear_dtr
                                          dm_put_device
                                           dm_put_table_device

However, in multipath_message(), the dm_dev is created and destroyed
under srcu_read_lock, which means destroying dm_dev in this process
and using dm_dev in other process will happen at same time since both
of them hold srcu_read_lock.

target_message
  dm_get_live_table // lock
   multipath_message
    dm_get_device // created
     // may be got by other processes
    dm_put_device // destroyed
     // may be used by other processes
  dm_put_live_table // unlock

We figured out some other solutions:
1) Judge refcount of dd under some lock before using dm_dev;
2) Get dd from list and use dm_dev under rcu;
3) Implement dm_get_device_xxx() with reference to dm_get_device()
for dm-mpath to avoid creating dd when find_device() failed.

Compared to solutions above, Luo Meng's patch may be more appropriate.
Any suggestions will be appreciated.

Li

> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel
>
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Li Lingfeng]

Friendly ping ...

Thanks,
Li

在 2023/5/18 20:11, lilingfeng (A) 写道:
>
> 在 2022/10/18 3:56, Mike Snitzer 写道:
>> On Mon, Oct 10 2022 at 10:41P -0400,
>> Luo Meng <luomeng@huaweicloud.com> wrote:
>>
>>> From: Luo Meng <luomeng12@huawei.com>
>>>
>>> If dm_get_device() create dd in multipath_message(),
>>> and then call table_deps() after dm_put_table_device(),
>>> it will lead to concurrency UAF bugs.
>>>
>>> One of the concurrency UAF can be shown as below:
>>>
>>>           (USE)                        |    (FREE)
>>>                                        |  target_message
>>>                                        |    multipath_message
>>>                                        |      dm_put_device
>>>                                        | dm_put_table_device #
>>>                                        |          kfree(td)  # 
>>> table_device *td
>>> ioctl # DM_TABLE_DEPS_CMD             |         ...
>>>    table_deps                          |         ...
>>>    dm_get_live_or_inactive_table       |         ...
>>>      retrieve_dep                      |         ...
>>>      list_for_each_entry               |         ...
>>>        deps->dev[count++] =            |         ...
>>>            huge_encode_dev             |         ...
>>>            (dd->dm_dev->bdev->bd_dev)  | list_del(&dd->list)
>>>                                        |        kfree(dd) # 
>>> dm_dev_internal
>>>
>>> The root cause of UAF bugs is that find_device() failed in
>>> dm_get_device() and will create dd and refcount set 1, kfree()
>>> in dm_put_table() is not protected. When td, which there are
>>> still pointers point to, is released, the concurrency UAF bug
>>> will happen.
>>>
>>> This patch add a flag to determine whether to create a new dd.
>>>
>>> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range 
>>> parameters)
>>> Signed-off-by: Luo Meng <luomeng12@huawei.com>
>>> ---
>>>   drivers/md/dm-mpath.c         |  2 +-
>>>   drivers/md/dm-table.c         | 43 
>>> +++++++++++++++++++++--------------
>>>   include/linux/device-mapper.h |  2 ++
>>>   3 files changed, 29 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>> index 0e325469a252..1f51059520ac 100644
>>> --- a/drivers/md/dm-mpath.c
>>> +++ b/drivers/md/dm-mpath.c
>>> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target 
>>> *ti, unsigned argc, char **argv,
>>>           goto out;
>>>       }
>>>   -    r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), 
>>> &dev);
>>> +    r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), 
>>> &dev, false);
>>>       if (r) {
>>>           DMWARN("message: error getting device %s",
>>>                  argv[1]);
>>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>>> index d8034ff0cb24..ad18a41f1608 100644
>>> --- a/drivers/md/dm-table.c
>>> +++ b/drivers/md/dm-table.c
>>> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
>>>   }
>>>   EXPORT_SYMBOL_GPL(dm_get_dev_t);
>>>   -/*
>>> - * Add a device to the list, or just increment the usage count if
>>> - * it's already present.
>>> - */
>>> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t 
>>> mode,
>>> -          struct dm_dev **result)
>>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t 
>>> mode,
>>> +            struct dm_dev **result, bool create_dd)
>>>   {
>>>       int r;
>>>       dev_t dev;
>>> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const 
>>> char *path, fmode_t mode,
>>>         dd = find_device(&t->devices, dev);
>>>       if (!dd) {
>>> -        dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>>> -        if (!dd)
>>> -            return -ENOMEM;
>>> -
>>> -        if ((r = dm_get_table_device(t->md, dev, mode, 
>>> &dd->dm_dev))) {
>>> -            kfree(dd);
>>> -            return r;
>>> -        }
>>> +        if (create_dd) {
>>> +            dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>>> +            if (!dd)
>>> +                return -ENOMEM;
>>>   -        refcount_set(&dd->count, 1);
>>> -        list_add(&dd->list, &t->devices);
>>> -        goto out;
>>> +            if ((r = dm_get_table_device(t->md, dev, mode, 
>>> &dd->dm_dev))) {
>>> +                kfree(dd);
>>> +                return r;
>>> +            }
>>>   +            refcount_set(&dd->count, 1);
>>> +            list_add(&dd->list, &t->devices);
>>> +            goto out;
>>> +        } else
>>> +            return -ENODEV;
>>>       } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>>>           r = upgrade_mode(dd, mode, t->md);
>>>           if (r)
>>> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const 
>>> char *path, fmode_t mode,
>>>       *result = dd->dm_dev;
>>>       return 0;
>>>   }
>>> +EXPORT_SYMBOL(__dm_get_device);
>>> +
>>> +/*
>>> + * Add a device to the list, or just increment the usage count if
>>> + * it's already present.
>>> + */
>>> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t 
>>> mode,
>>> +          struct dm_dev **result)
>>> +{
>>> +    return __dm_get_device(ti, path, mode, result, true);
>>> +}
>>>   EXPORT_SYMBOL(dm_get_device);
>>>     static int dm_set_device_limits(struct dm_target *ti, struct 
>>> dm_dev *dev,
>>> diff --git a/include/linux/device-mapper.h 
>>> b/include/linux/device-mapper.h
>>> index 04c6acf7faaa..ef4031a844b6 100644
>>> --- a/include/linux/device-mapper.h
>>> +++ b/include/linux/device-mapper.h
>>> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
>>>   int dm_get_device(struct dm_target *ti, const char *path, fmode_t 
>>> mode,
>>>             struct dm_dev **result);
>>>   void dm_put_device(struct dm_target *ti, struct dm_dev *d);
>>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t 
>>> mode,
>>> +            struct dm_dev **result, bool create_dd);
>>>     /*
>>>    * Information about a target type
>>> -- 
>>> 2.31.1
>>>
>> This patch is treating the one multipath case like it is only consumer
>> of dm_get_device() that might have this issue. It feels too focused on
>> an instance where dm_get_device()'s side-effect of creating the device
>> is unwelcome. Feels like you're treating the symptom rather than the
>> problem (e.g. that the "kfree() in dm_put_table() is not protected"?)
>>
>> Mike
>
> In other cases, kfree() in dm_put_table() is protected by srcu.
> For example:
>           USE                              FREE
> table_deps                            dev_remove
>  dm_get_live_or_inactive_table         dm_sync_table
>   // lock
>   srcu_read_lock(&md->io_barrier)
>                                         // wait for unlock
> synchronize_srcu(&md->io_barrier)
>   retrieve_deps
>  dm_put_live_table
>   // unlock
>   srcu_read_unlock(&md->io_barrier...)
>                                        dm_table_destroy
>                                         linear_dtr
>                                          dm_put_device
>                                           dm_put_table_device
>
> However, in multipath_message(), the dm_dev is created and destroyed
> under srcu_read_lock, which means destroying dm_dev in this process
> and using dm_dev in other process will happen at same time since both
> of them hold srcu_read_lock.
>
> target_message
>  dm_get_live_table // lock
>   multipath_message
>    dm_get_device // created
>     // may be got by other processes
>    dm_put_device // destroyed
>     // may be used by other processes
>  dm_put_live_table // unlock
>
> We figured out some other solutions:
> 1) Judge refcount of dd under some lock before using dm_dev;
> 2) Get dd from list and use dm_dev under rcu;
> 3) Implement dm_get_device_xxx() with reference to dm_get_device()
> for dm-mpath to avoid creating dd when find_device() failed.
>
> Compared to solutions above, Luo Meng's patch may be more appropriate.
> Any suggestions will be appreciated.
>
> Li
>
>> -- 
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://listman.redhat.com/mailman/listinfo/dm-devel
>>
>>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Mike Snitzer]

On Thu, May 18 2023 at  8:11P -0400,
lilingfeng (A) <lilingfeng3@huawei.com> wrote:

> 
> 在 2022/10/18 3:56, Mike Snitzer 写道:
> > On Mon, Oct 10 2022 at 10:41P -0400,
> > Luo Meng <luomeng@huaweicloud.com> wrote:
> > 
> > > From: Luo Meng <luomeng12@huawei.com>
> > > 
> > > If dm_get_device() create dd in multipath_message(),
> > > and then call table_deps() after dm_put_table_device(),
> > > it will lead to concurrency UAF bugs.
> > > 
> > > One of the concurrency UAF can be shown as below:
> > > 
> > >           (USE)                        |    (FREE)
> > >                                        |  target_message
> > >                                        |    multipath_message
> > >                                        |      dm_put_device
> > >                                        |        dm_put_table_device #
> > >                                        |          kfree(td)  # table_device *td
> > > ioctl # DM_TABLE_DEPS_CMD             |         ...
> > >    table_deps                          |         ...
> > >    dm_get_live_or_inactive_table       |         ...
> > >      retrieve_dep                      |         ...
> > >      list_for_each_entry               |         ...
> > >        deps->dev[count++] =            |         ...
> > >            huge_encode_dev             |         ...
> > >            (dd->dm_dev->bdev->bd_dev)  |        list_del(&dd->list)
> > >                                        |        kfree(dd) # dm_dev_internal
> > > 
> > > The root cause of UAF bugs is that find_device() failed in
> > > dm_get_device() and will create dd and refcount set 1, kfree()
> > > in dm_put_table() is not protected. When td, which there are
> > > still pointers point to, is released, the concurrency UAF bug
> > > will happen.
> > > 
> > > This patch add a flag to determine whether to create a new dd.
> > > 
> > > Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
> > > Signed-off-by: Luo Meng <luomeng12@huawei.com>
> > > ---
> > >   drivers/md/dm-mpath.c         |  2 +-
> > >   drivers/md/dm-table.c         | 43 +++++++++++++++++++++--------------
> > >   include/linux/device-mapper.h |  2 ++
> > >   3 files changed, 29 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > > index 0e325469a252..1f51059520ac 100644
> > > --- a/drivers/md/dm-mpath.c
> > > +++ b/drivers/md/dm-mpath.c
> > > @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
> > >   		goto out;
> > >   	}
> > > -	r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
> > > +	r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
> > >   	if (r) {
> > >   		DMWARN("message: error getting device %s",
> > >   		       argv[1]);
> > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > index d8034ff0cb24..ad18a41f1608 100644
> > > --- a/drivers/md/dm-table.c
> > > +++ b/drivers/md/dm-table.c
> > > @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
> > >   }
> > >   EXPORT_SYMBOL_GPL(dm_get_dev_t);
> > > -/*
> > > - * Add a device to the list, or just increment the usage count if
> > > - * it's already present.
> > > - */
> > > -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > -		  struct dm_dev **result)
> > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > +		    struct dm_dev **result, bool create_dd)
> > >   {
> > >   	int r;
> > >   	dev_t dev;
> > > @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > >   	dd = find_device(&t->devices, dev);
> > >   	if (!dd) {
> > > -		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > -		if (!dd)
> > > -			return -ENOMEM;
> > > -
> > > -		if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> > > -			kfree(dd);
> > > -			return r;
> > > -		}
> > > +		if (create_dd) {
> > > +			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > +			if (!dd)
> > > +				return -ENOMEM;
> > > -		refcount_set(&dd->count, 1);
> > > -		list_add(&dd->list, &t->devices);
> > > -		goto out;
> > > +			if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> > > +				kfree(dd);
> > > +				return r;
> > > +			}
> > > +			refcount_set(&dd->count, 1);
> > > +			list_add(&dd->list, &t->devices);
> > > +			goto out;
> > > +		} else
> > > +			return -ENODEV;
> > >   	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> > >   		r = upgrade_mode(dd, mode, t->md);
> > >   		if (r)
> > > @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > >   	*result = dd->dm_dev;
> > >   	return 0;
> > >   }
> > > +EXPORT_SYMBOL(__dm_get_device);
> > > +
> > > +/*
> > > + * Add a device to the list, or just increment the usage count if
> > > + * it's already present.
> > > + */
> > > +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > +		  struct dm_dev **result)
> > > +{
> > > +	return __dm_get_device(ti, path, mode, result, true);
> > > +}
> > >   EXPORT_SYMBOL(dm_get_device);
> > >   static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
> > > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
> > > index 04c6acf7faaa..ef4031a844b6 100644
> > > --- a/include/linux/device-mapper.h
> > > +++ b/include/linux/device-mapper.h
> > > @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
> > >   int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > >   		  struct dm_dev **result);
> > >   void dm_put_device(struct dm_target *ti, struct dm_dev *d);
> > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > +		    struct dm_dev **result, bool create_dd);
> > >   /*
> > >    * Information about a target type
> > > -- 
> > > 2.31.1
> > > 
> > This patch is treating the one multipath case like it is only consumer
> > of dm_get_device() that might have this issue. It feels too focused on
> > an instance where dm_get_device()'s side-effect of creating the device
> > is unwelcome. Feels like you're treating the symptom rather than the
> > problem (e.g. that the "kfree() in dm_put_table() is not protected"?)
> > 
> > Mike
> 
> In other cases, kfree() in dm_put_table() is protected by srcu.
> For example:
>           USE                              FREE
> table_deps                            dev_remove
>  dm_get_live_or_inactive_table         dm_sync_table
>   // lock
>   srcu_read_lock(&md->io_barrier)
>                                         // wait for unlock
>                                         synchronize_srcu(&md->io_barrier)
>   retrieve_deps
>  dm_put_live_table
>   // unlock
>   srcu_read_unlock(&md->io_barrier...)
>                                        dm_table_destroy
>                                         linear_dtr
>                                          dm_put_device
>                                           dm_put_table_device
> 
> However, in multipath_message(), the dm_dev is created and destroyed
> under srcu_read_lock, which means destroying dm_dev in this process
> and using dm_dev in other process will happen at same time since both
> of them hold srcu_read_lock.
> 
> target_message
>  dm_get_live_table // lock
>   multipath_message
>    dm_get_device // created
>     // may be got by other processes
>    dm_put_device // destroyed
>     // may be used by other processes
>  dm_put_live_table // unlock
> 
> We figured out some other solutions:
> 1) Judge refcount of dd under some lock before using dm_dev;
> 2) Get dd from list and use dm_dev under rcu;
> 3) Implement dm_get_device_xxx() with reference to dm_get_device()
> for dm-mpath to avoid creating dd when find_device() failed.
> 
> Compared to solutions above, Luo Meng's patch may be more appropriate.
> Any suggestions will be appreciated.
> 
> Li

[Cc'ing Mikulas so he can take a look at this too.]

The proposed patch papers over the issue, leaving a landmine for some
future DM target.

In addition, the patch header is very muddled about relation between
table_device and dm_dev_internal. And also needs clarification like:
"kfree(td) in dm_put_table_device() is not interlocked with
dm_dev_internal list (table->devices) management"

Also, the commit referenced with the "Fixes:" is bogus.

Wouldn't this change address the UAF (albeit, list_for_each_entry_safe
likely needed in methods like retrieve_deps())?

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 7d208b2b1a19..39e4c9ee8f16 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -434,9 +434,9 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d)
 		return;
 	}
 	if (refcount_dec_and_test(&dd->count)) {
-		dm_put_table_device(ti->table->md, d);
 		list_del(&dd->list);
 		kfree(dd);
+		dm_put_table_device(ti->table->md, d);
 	}
 }
 EXPORT_SYMBOL(dm_put_device);

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Li Lingfeng]

在 2023/8/3 0:33, Mike Snitzer 写道:
> On Thu, May 18 2023 at  8:11P -0400,
> lilingfeng (A) <lilingfeng3@huawei.com> wrote:
>
>> 在 2022/10/18 3:56, Mike Snitzer 写道:
>>> On Mon, Oct 10 2022 at 10:41P -0400,
>>> Luo Meng <luomeng@huaweicloud.com> wrote:
>>>
>>>> From: Luo Meng <luomeng12@huawei.com>
>>>>
>>>> If dm_get_device() create dd in multipath_message(),
>>>> and then call table_deps() after dm_put_table_device(),
>>>> it will lead to concurrency UAF bugs.
>>>>
>>>> One of the concurrency UAF can be shown as below:
>>>>
>>>>            (USE)                        |    (FREE)
>>>>                                         |  target_message
>>>>                                         |    multipath_message
>>>>                                         |      dm_put_device
>>>>                                         |        dm_put_table_device #
>>>>                                         |          kfree(td)  # table_device *td
>>>> ioctl # DM_TABLE_DEPS_CMD             |         ...
>>>>     table_deps                          |         ...
>>>>     dm_get_live_or_inactive_table       |         ...
>>>>       retrieve_dep                      |         ...
>>>>       list_for_each_entry               |         ...
>>>>         deps->dev[count++] =            |         ...
>>>>             huge_encode_dev             |         ...
>>>>             (dd->dm_dev->bdev->bd_dev)  |        list_del(&dd->list)
>>>>                                         |        kfree(dd) # dm_dev_internal
>>>>
>>>> The root cause of UAF bugs is that find_device() failed in
>>>> dm_get_device() and will create dd and refcount set 1, kfree()
>>>> in dm_put_table() is not protected. When td, which there are
>>>> still pointers point to, is released, the concurrency UAF bug
>>>> will happen.
>>>>
>>>> This patch add a flag to determine whether to create a new dd.
>>>>
>>>> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
>>>> Signed-off-by: Luo Meng <luomeng12@huawei.com>
>>>> ---
>>>>    drivers/md/dm-mpath.c         |  2 +-
>>>>    drivers/md/dm-table.c         | 43 +++++++++++++++++++++--------------
>>>>    include/linux/device-mapper.h |  2 ++
>>>>    3 files changed, 29 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>>> index 0e325469a252..1f51059520ac 100644
>>>> --- a/drivers/md/dm-mpath.c
>>>> +++ b/drivers/md/dm-mpath.c
>>>> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
>>>>    		goto out;
>>>>    	}
>>>> -	r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
>>>> +	r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
>>>>    	if (r) {
>>>>    		DMWARN("message: error getting device %s",
>>>>    		       argv[1]);
>>>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>>>> index d8034ff0cb24..ad18a41f1608 100644
>>>> --- a/drivers/md/dm-table.c
>>>> +++ b/drivers/md/dm-table.c
>>>> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(dm_get_dev_t);
>>>> -/*
>>>> - * Add a device to the list, or just increment the usage count if
>>>> - * it's already present.
>>>> - */
>>>> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>>> -		  struct dm_dev **result)
>>>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>>> +		    struct dm_dev **result, bool create_dd)
>>>>    {
>>>>    	int r;
>>>>    	dev_t dev;
>>>> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>>>    	dd = find_device(&t->devices, dev);
>>>>    	if (!dd) {
>>>> -		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>>>> -		if (!dd)
>>>> -			return -ENOMEM;
>>>> -
>>>> -		if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
>>>> -			kfree(dd);
>>>> -			return r;
>>>> -		}
>>>> +		if (create_dd) {
>>>> +			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
>>>> +			if (!dd)
>>>> +				return -ENOMEM;
>>>> -		refcount_set(&dd->count, 1);
>>>> -		list_add(&dd->list, &t->devices);
>>>> -		goto out;
>>>> +			if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
>>>> +				kfree(dd);
>>>> +				return r;
>>>> +			}
>>>> +			refcount_set(&dd->count, 1);
>>>> +			list_add(&dd->list, &t->devices);
>>>> +			goto out;
>>>> +		} else
>>>> +			return -ENODEV;
>>>>    	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>>>>    		r = upgrade_mode(dd, mode, t->md);
>>>>    		if (r)
>>>> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>>>    	*result = dd->dm_dev;
>>>>    	return 0;
>>>>    }
>>>> +EXPORT_SYMBOL(__dm_get_device);
>>>> +
>>>> +/*
>>>> + * Add a device to the list, or just increment the usage count if
>>>> + * it's already present.
>>>> + */
>>>> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>>> +		  struct dm_dev **result)
>>>> +{
>>>> +	return __dm_get_device(ti, path, mode, result, true);
>>>> +}
>>>>    EXPORT_SYMBOL(dm_get_device);
>>>>    static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
>>>> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
>>>> index 04c6acf7faaa..ef4031a844b6 100644
>>>> --- a/include/linux/device-mapper.h
>>>> +++ b/include/linux/device-mapper.h
>>>> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
>>>>    int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>>>    		  struct dm_dev **result);
>>>>    void dm_put_device(struct dm_target *ti, struct dm_dev *d);
>>>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
>>>> +		    struct dm_dev **result, bool create_dd);
>>>>    /*
>>>>     * Information about a target type
>>>> -- 
>>>> 2.31.1
>>>>
>>> This patch is treating the one multipath case like it is only consumer
>>> of dm_get_device() that might have this issue. It feels too focused on
>>> an instance where dm_get_device()'s side-effect of creating the device
>>> is unwelcome. Feels like you're treating the symptom rather than the
>>> problem (e.g. that the "kfree() in dm_put_table() is not protected"?)
>>>
>>> Mike
>> In other cases, kfree() in dm_put_table() is protected by srcu.
>> For example:
>>            USE                              FREE
>> table_deps                            dev_remove
>>   dm_get_live_or_inactive_table         dm_sync_table
>>    // lock
>>    srcu_read_lock(&md->io_barrier)
>>                                          // wait for unlock
>>                                          synchronize_srcu(&md->io_barrier)
>>    retrieve_deps
>>   dm_put_live_table
>>    // unlock
>>    srcu_read_unlock(&md->io_barrier...)
>>                                         dm_table_destroy
>>                                          linear_dtr
>>                                           dm_put_device
>>                                            dm_put_table_device
>>
>> However, in multipath_message(), the dm_dev is created and destroyed
>> under srcu_read_lock, which means destroying dm_dev in this process
>> and using dm_dev in other process will happen at same time since both
>> of them hold srcu_read_lock.
>>
>> target_message
>>   dm_get_live_table // lock
>>    multipath_message
>>     dm_get_device // created
>>      // may be got by other processes
>>     dm_put_device // destroyed
>>      // may be used by other processes
>>   dm_put_live_table // unlock
>>
>> We figured out some other solutions:
>> 1) Judge refcount of dd under some lock before using dm_dev;
>> 2) Get dd from list and use dm_dev under rcu;
>> 3) Implement dm_get_device_xxx() with reference to dm_get_device()
>> for dm-mpath to avoid creating dd when find_device() failed.
>>
>> Compared to solutions above, Luo Meng's patch may be more appropriate.
>> Any suggestions will be appreciated.
>>
>> Li
> [Cc'ing Mikulas so he can take a look at this too.]
>
> The proposed patch papers over the issue, leaving a landmine for some
> future DM target.
>
> In addition, the patch header is very muddled about relation between
> table_device and dm_dev_internal. And also needs clarification like:
> "kfree(td) in dm_put_table_device() is not interlocked with
> dm_dev_internal list (table->devices) management"
>
> Also, the commit referenced with the "Fixes:" is bogus.
>
> Wouldn't this change address the UAF (albeit, list_for_each_entry_safe
> likely needed in methods like retrieve_deps())?
>
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 7d208b2b1a19..39e4c9ee8f16 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -434,9 +434,9 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d)
>   		return;
>   	}
>   	if (refcount_dec_and_test(&dd->count)) {
> -		dm_put_table_device(ti->table->md, d);
>   		list_del(&dd->list);
>   		kfree(dd);
> +		dm_put_table_device(ti->table->md, d);
>   	}
>   }
>   EXPORT_SYMBOL(dm_put_device);
Thanks for your reply.

Deleting dm_dev_internal from the list before freeing table_device may not
fix the problem since the process of "USE" may have got dm_dev_internal
before deleting and try to use it after freeing.

          (USE)                        |    (FREE)
                                       |  target_message
                                       |    multipath_message
                                       |      dm_put_device
ioctl # DM_TABLE_DEPS_CMD             |         ...
   table_deps                          |         ...
   dm_get_live_or_inactive_table       |         ...
     retrieve_dep                      |         ...
     list_for_each_entry  ---- got dd  |         ...
       deps->dev[count++] =            |         ...
                                       |        list_del(&dd->list) ---- delete
                                       |        kfree(dd) # dm_dev_internal
                                       |        dm_put_table_device #
                                       |          kfree(td)  ---- free
           huge_encode_dev             |         ...
           (dd->dm_dev->bdev->bd_dev)  |
            ----> UAF

Li

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 12739 bytes --]



On Wed, 2 Aug 2023, Mike Snitzer wrote:

> On Thu, May 18 2023 at  8:11P -0400,
> lilingfeng (A) <lilingfeng3@huawei.com> wrote:
> 
> > 
> > 在 2022/10/18 3:56, Mike Snitzer 写道:
> > > On Mon, Oct 10 2022 at 10:41P -0400,
> > > Luo Meng <luomeng@huaweicloud.com> wrote:
> > > 
> > > > From: Luo Meng <luomeng12@huawei.com>
> > > > 
> > > > If dm_get_device() create dd in multipath_message(),
> > > > and then call table_deps() after dm_put_table_device(),
> > > > it will lead to concurrency UAF bugs.
> > > > 
> > > > One of the concurrency UAF can be shown as below:
> > > > 
> > > >           (USE)                        |    (FREE)
> > > >                                        |  target_message
> > > >                                        |    multipath_message
> > > >                                        |      dm_put_device
> > > >                                        |        dm_put_table_device #
> > > >                                        |          kfree(td)  # table_device *td
> > > > ioctl # DM_TABLE_DEPS_CMD             |         ...
> > > >    table_deps                          |         ...
> > > >    dm_get_live_or_inactive_table       |         ...
> > > >      retrieve_dep                      |         ...
> > > >      list_for_each_entry               |         ...
> > > >        deps->dev[count++] =            |         ...
> > > >            huge_encode_dev             |         ...
> > > >            (dd->dm_dev->bdev->bd_dev)  |        list_del(&dd->list)
> > > >                                        |        kfree(dd) # dm_dev_internal
> > > > 
> > > > The root cause of UAF bugs is that find_device() failed in
> > > > dm_get_device() and will create dd and refcount set 1, kfree()
> > > > in dm_put_table() is not protected. When td, which there are
> > > > still pointers point to, is released, the concurrency UAF bug
> > > > will happen.
> > > > 
> > > > This patch add a flag to determine whether to create a new dd.
> > > > 
> > > > Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
> > > > Signed-off-by: Luo Meng <luomeng12@huawei.com>
> > > > ---
> > > >   drivers/md/dm-mpath.c         |  2 +-
> > > >   drivers/md/dm-table.c         | 43 +++++++++++++++++++++--------------
> > > >   include/linux/device-mapper.h |  2 ++
> > > >   3 files changed, 29 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > > > index 0e325469a252..1f51059520ac 100644
> > > > --- a/drivers/md/dm-mpath.c
> > > > +++ b/drivers/md/dm-mpath.c
> > > > @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
> > > >   		goto out;
> > > >   	}
> > > > -	r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
> > > > +	r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
> > > >   	if (r) {
> > > >   		DMWARN("message: error getting device %s",
> > > >   		       argv[1]);
> > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > > index d8034ff0cb24..ad18a41f1608 100644
> > > > --- a/drivers/md/dm-table.c
> > > > +++ b/drivers/md/dm-table.c
> > > > @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(dm_get_dev_t);
> > > > -/*
> > > > - * Add a device to the list, or just increment the usage count if
> > > > - * it's already present.
> > > > - */
> > > > -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > -		  struct dm_dev **result)
> > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > +		    struct dm_dev **result, bool create_dd)
> > > >   {
> > > >   	int r;
> > > >   	dev_t dev;
> > > > @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > >   	dd = find_device(&t->devices, dev);
> > > >   	if (!dd) {
> > > > -		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > > -		if (!dd)
> > > > -			return -ENOMEM;
> > > > -
> > > > -		if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> > > > -			kfree(dd);
> > > > -			return r;
> > > > -		}
> > > > +		if (create_dd) {
> > > > +			dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > > +			if (!dd)
> > > > +				return -ENOMEM;
> > > > -		refcount_set(&dd->count, 1);
> > > > -		list_add(&dd->list, &t->devices);
> > > > -		goto out;
> > > > +			if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> > > > +				kfree(dd);
> > > > +				return r;
> > > > +			}
> > > > +			refcount_set(&dd->count, 1);
> > > > +			list_add(&dd->list, &t->devices);
> > > > +			goto out;
> > > > +		} else
> > > > +			return -ENODEV;
> > > >   	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> > > >   		r = upgrade_mode(dd, mode, t->md);
> > > >   		if (r)
> > > > @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > >   	*result = dd->dm_dev;
> > > >   	return 0;
> > > >   }
> > > > +EXPORT_SYMBOL(__dm_get_device);
> > > > +
> > > > +/*
> > > > + * Add a device to the list, or just increment the usage count if
> > > > + * it's already present.
> > > > + */
> > > > +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > +		  struct dm_dev **result)
> > > > +{
> > > > +	return __dm_get_device(ti, path, mode, result, true);
> > > > +}
> > > >   EXPORT_SYMBOL(dm_get_device);
> > > >   static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
> > > > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
> > > > index 04c6acf7faaa..ef4031a844b6 100644
> > > > --- a/include/linux/device-mapper.h
> > > > +++ b/include/linux/device-mapper.h
> > > > @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
> > > >   int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > >   		  struct dm_dev **result);
> > > >   void dm_put_device(struct dm_target *ti, struct dm_dev *d);
> > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > +		    struct dm_dev **result, bool create_dd);
> > > >   /*
> > > >    * Information about a target type
> > > > -- 
> > > > 2.31.1
> > > > 
> > > This patch is treating the one multipath case like it is only consumer
> > > of dm_get_device() that might have this issue. It feels too focused on
> > > an instance where dm_get_device()'s side-effect of creating the device
> > > is unwelcome. Feels like you're treating the symptom rather than the
> > > problem (e.g. that the "kfree() in dm_put_table() is not protected"?)
> > > 
> > > Mike
> > 
> > In other cases, kfree() in dm_put_table() is protected by srcu.
> > For example:
> >           USE                              FREE
> > table_deps                            dev_remove
> >  dm_get_live_or_inactive_table         dm_sync_table
> >   // lock
> >   srcu_read_lock(&md->io_barrier)
> >                                         // wait for unlock
> >                                         synchronize_srcu(&md->io_barrier)
> >   retrieve_deps
> >  dm_put_live_table
> >   // unlock
> >   srcu_read_unlock(&md->io_barrier...)
> >                                        dm_table_destroy
> >                                         linear_dtr
> >                                          dm_put_device
> >                                           dm_put_table_device
> > 
> > However, in multipath_message(), the dm_dev is created and destroyed
> > under srcu_read_lock, which means destroying dm_dev in this process
> > and using dm_dev in other process will happen at same time since both
> > of them hold srcu_read_lock.
> > 
> > target_message
> >  dm_get_live_table // lock
> >   multipath_message
> >    dm_get_device // created
> >     // may be got by other processes
> >    dm_put_device // destroyed
> >     // may be used by other processes
> >  dm_put_live_table // unlock
> > 
> > We figured out some other solutions:
> > 1) Judge refcount of dd under some lock before using dm_dev;
> > 2) Get dd from list and use dm_dev under rcu;
> > 3) Implement dm_get_device_xxx() with reference to dm_get_device()
> > for dm-mpath to avoid creating dd when find_device() failed.
> > 
> > Compared to solutions above, Luo Meng's patch may be more appropriate.
> > Any suggestions will be appreciated.
> > 
> > Li
> 
> [Cc'ing Mikulas so he can take a look at this too.]

Hi

I suggest this patch (but it's only compile-tested, so please run some 
testsuite on it).

Mikulas


From: Mikulas Patocka <mpatocka@redhat.com>
Subject: [PATCH] dm: fix a race condition in retrieve_deps

There's a race condition in the multipath target when retrieve_deps races
with multipath_message. multipath_message opens and closes a device using
dm_get_device and dm_put_device; retrieve_deps walks the list of open
devices without holding any lock. The end result may be memory corruption
or use-after-free memory access.

multipath_message already holds a mutex that prevents two
multipath_messages from running concurrently - in order to fix this race
condition, we modify retrieve_deps, so that it grabs and frees this mutex
too.

We add an entry "devices_mutex" to "struct dm_table" and we introduce a 
function "dm_table_set_devices_mutex" that sets it. The mutex is set in 
the multipath target contructor.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/md/dm-core.h          |    5 +++++
 drivers/md/dm-ioctl.c         |    9 ++++++++-
 drivers/md/dm-mpath.c         |    2 ++
 drivers/md/dm-table.c         |    6 ++++++
 include/linux/device-mapper.h |    5 +++++
 5 files changed, 26 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/md/dm-core.h
===================================================================
--- linux-2.6.orig/drivers/md/dm-core.h
+++ linux-2.6/drivers/md/dm-core.h
@@ -214,6 +214,11 @@ struct dm_table {
 
 	/* a list of devices used by this table */
 	struct list_head devices;
+	/*
+	 * The mutex should be set if the target modifies the "devices" list
+	 * outside of the constructor and destructor.
+	 */
+	struct mutex *devices_mutex;
 
 	/* events get handed up using this callback */
 	void (*event_fn)(void *data);
Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c
+++ linux-2.6/drivers/md/dm-table.c
@@ -2034,6 +2034,12 @@ struct list_head *dm_table_get_devices(s
 	return &t->devices;
 }
 
+void dm_table_set_devices_mutex(struct dm_table *t, struct mutex *m)
+{
+	t->devices_mutex = m;
+}
+EXPORT_SYMBOL(dm_table_set_devices_mutex);
+
 blk_mode_t dm_table_get_mode(struct dm_table *t)
 {
 	return t->mode;
Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-ioctl.c
+++ linux-2.6/drivers/md/dm-ioctl.c
@@ -1630,6 +1630,9 @@ static void retrieve_deps(struct dm_tabl
 	struct dm_dev_internal *dd;
 	struct dm_target_deps *deps;
 
+	if (table->devices_mutex)
+		mutex_lock(table->devices_mutex);
+
 	deps = get_result_buffer(param, param_size, &len);
 
 	/*
@@ -1644,7 +1647,7 @@ static void retrieve_deps(struct dm_tabl
 	needed = struct_size(deps, dev, count);
 	if (len < needed) {
 		param->flags |= DM_BUFFER_FULL_FLAG;
-		return;
+		goto ret;
 	}
 
 	/*
@@ -1656,6 +1659,10 @@ static void retrieve_deps(struct dm_tabl
 		deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev);
 
 	param->data_size = param->data_start + needed;
+
+ret:
+	if (table->devices_mutex)
+		mutex_unlock(table->devices_mutex);
 }
 
 static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size)
Index: linux-2.6/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-mpath.c
+++ linux-2.6/drivers/md/dm-mpath.c
@@ -1268,6 +1268,8 @@ static int multipath_ctr(struct dm_targe
 	else
 		ti->per_io_data_size = sizeof(struct dm_mpath_io);
 
+	dm_table_set_devices_mutex(ti->table, &m->work_mutex);
+
 	return 0;
 
  bad:
Index: linux-2.6/include/linux/device-mapper.h
===================================================================
--- linux-2.6.orig/include/linux/device-mapper.h
+++ linux-2.6/include/linux/device-mapper.h
@@ -177,6 +177,11 @@ struct dm_dev {
 int dm_get_device(struct dm_target *ti, const char *path, blk_mode_t mode,
 		  struct dm_dev **result);
 void dm_put_device(struct dm_target *ti, struct dm_dev *d);
+/*
+ * The mutex must be set if we use dm_get_device or dm_put_device outside
+ * of the constructor and destructor.
+ */
+void dm_table_set_devices_mutex(struct dm_table *t, struct mutex *m);
 
 /*
  * Information about a target type

[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Mikulas Patocka]

On Tue, 8 Aug 2023, Mikulas Patocka wrote:

> On Wed, 2 Aug 2023, Mike Snitzer wrote:
> 
> > [Cc'ing Mikulas so he can take a look at this too.]
> 
> Hi
> 
> I suggest this patch (but it's only compile-tested, so please run some 
> testsuite on it).
> 
> Mikulas

I self-nack that patch - it doesn't work if there are multiple targets 
calling dm_table_set_devices_mutex in the same table. This is not an issue 
for multipath, but it will be a problem if other targets use this 
functionality.

Here I'm sending a better patch that doesn't need any modification to the 
targets at all.

Mikulas



From: Mikulas Patocka <mpatocka at redhat.com>
Subject: [PATCH] dm: fix a race condition in retrieve_deps

There's a race condition in the multipath target when retrieve_deps races
with multipath_message calling dm_get_device and dm_put_device.
retrieve_deps walks the list of open devices without holding any lock but
multipath may add or remove devices to the list while it is running. The
end result may be memory corruption or use-after-free memory access.

Fix this bug by introducing a new rw semaphore "devices_lock". We grab
devices_lock for read in retrieve_deps and we grab it for write in
dm_get_device and dm_put_device.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/md/dm-core.h  |    1 +
 drivers/md/dm-ioctl.c |    7 ++++++-
 drivers/md/dm-table.c |   32 ++++++++++++++++++++++++--------
 3 files changed, 31 insertions(+), 9 deletions(-)

Index: linux-2.6/drivers/md/dm-core.h
===================================================================
--- linux-2.6.orig/drivers/md/dm-core.h
+++ linux-2.6/drivers/md/dm-core.h
@@ -214,6 +214,7 @@ struct dm_table {
 
 	/* a list of devices used by this table */
 	struct list_head devices;
+	struct rw_semaphore devices_lock;
 
 	/* events get handed up using this callback */
 	void (*event_fn)(void *data);
Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-ioctl.c
+++ linux-2.6/drivers/md/dm-ioctl.c
@@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl
 	struct dm_dev_internal *dd;
 	struct dm_target_deps *deps;
 
+	down_read(&table->devices_lock);
+
 	deps = get_result_buffer(param, param_size, &len);
 
 	/*
@@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl
 	needed = struct_size(deps, dev, count);
 	if (len < needed) {
 		param->flags |= DM_BUFFER_FULL_FLAG;
-		return;
+		goto out;
 	}
 
 	/*
@@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl
 		deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev);
 
 	param->data_size = param->data_start + needed;
+
+out:
+	up_read(&table->devices_lock);
 }
 
 static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size)
Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c
+++ linux-2.6/drivers/md/dm-table.c
@@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&t->devices);
+	init_rwsem(&t->devices_lock);
 
 	if (!num_targets)
 		num_targets = KEYS_PER_NODE;
@@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target
 	if (dev == disk_devt(t->md->disk))
 		return -EINVAL;
 
+	down_write(&t->devices_lock);
+
 	dd = find_device(&t->devices, dev);
 	if (!dd) {
 		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
-		if (!dd)
-			return -ENOMEM;
+		if (!dd) {
+			r = -ENOMEM;
+			goto unlock_ret_r;
+		}
 
 		r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev);
 		if (r) {
 			kfree(dd);
-			return r;
+			goto unlock_ret_r;
 		}
 
 		refcount_set(&dd->count, 1);
@@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target
 	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
 		r = upgrade_mode(dd, mode, t->md);
 		if (r)
-			return r;
+			goto unlock_ret_r;
 	}
 	refcount_inc(&dd->count);
 out:
+	up_write(&t->devices_lock);
 	*result = dd->dm_dev;
 	return 0;
+
+unlock_ret_r:
+	up_write(&t->devices_lock);
+	return r;
 }
 EXPORT_SYMBOL(dm_get_device);
 
@@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d
 void dm_put_device(struct dm_target *ti, struct dm_dev *d)
 {
 	int found = 0;
-	struct list_head *devices = &ti->table->devices;
+	struct dm_table *t = ti->table;
+	struct list_head *devices = &t->devices;
 	struct dm_dev_internal *dd;
 
+	down_write(&t->devices_lock);
+
 	list_for_each_entry(dd, devices, list) {
 		if (dd->dm_dev == d) {
 			found = 1;
@@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti,
 	}
 	if (!found) {
 		DMERR("%s: device %s not in table devices list",
-		      dm_device_name(ti->table->md), d->name);
-		return;
+		      dm_device_name(t->md), d->name);
+		goto unlock_ret;
 	}
 	if (refcount_dec_and_test(&dd->count)) {
-		dm_put_table_device(ti->table->md, d);
+		dm_put_table_device(t->md, d);
 		list_del(&dd->list);
 		kfree(dd);
 	}
+
+unlock_ret:
+	up_write(&t->devices_lock);
 }
 EXPORT_SYMBOL(dm_put_device);
 
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Li Lingfeng]

Hi

Thanks to Mikulas for the patch. I have test the patch and it can fix 
the problem.
Can this patch be applied to mainline?

Thanks.

在 2023/8/9 18:44, Mikulas Patocka 写道:
>
> On Tue, 8 Aug 2023, Mikulas Patocka wrote:
>
>> On Wed, 2 Aug 2023, Mike Snitzer wrote:
>>
>>> [Cc'ing Mikulas so he can take a look at this too.]
>> Hi
>>
>> I suggest this patch (but it's only compile-tested, so please run some
>> testsuite on it).
>>
>> Mikulas
> I self-nack that patch - it doesn't work if there are multiple targets
> calling dm_table_set_devices_mutex in the same table. This is not an issue
> for multipath, but it will be a problem if other targets use this
> functionality.
>
> Here I'm sending a better patch that doesn't need any modification to the
> targets at all.
>
> Mikulas
>
>
>
> From: Mikulas Patocka <mpatocka at redhat.com>
> Subject: [PATCH] dm: fix a race condition in retrieve_deps
>
> There's a race condition in the multipath target when retrieve_deps races
> with multipath_message calling dm_get_device and dm_put_device.
> retrieve_deps walks the list of open devices without holding any lock but
> multipath may add or remove devices to the list while it is running. The
> end result may be memory corruption or use-after-free memory access.
>
> Fix this bug by introducing a new rw semaphore "devices_lock". We grab
> devices_lock for read in retrieve_deps and we grab it for write in
> dm_get_device and dm_put_device.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
>
> ---
>   drivers/md/dm-core.h  |    1 +
>   drivers/md/dm-ioctl.c |    7 ++++++-
>   drivers/md/dm-table.c |   32 ++++++++++++++++++++++++--------
>   3 files changed, 31 insertions(+), 9 deletions(-)
>
> Index: linux-2.6/drivers/md/dm-core.h
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-core.h
> +++ linux-2.6/drivers/md/dm-core.h
> @@ -214,6 +214,7 @@ struct dm_table {
>   
>   	/* a list of devices used by this table */
>   	struct list_head devices;
> +	struct rw_semaphore devices_lock;
>   
>   	/* events get handed up using this callback */
>   	void (*event_fn)(void *data);
> Index: linux-2.6/drivers/md/dm-ioctl.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-ioctl.c
> +++ linux-2.6/drivers/md/dm-ioctl.c
> @@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl
>   	struct dm_dev_internal *dd;
>   	struct dm_target_deps *deps;
>   
> +	down_read(&table->devices_lock);
> +
>   	deps = get_result_buffer(param, param_size, &len);
>   
>   	/*
> @@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl
>   	needed = struct_size(deps, dev, count);
>   	if (len < needed) {
>   		param->flags |= DM_BUFFER_FULL_FLAG;
> -		return;
> +		goto out;
>   	}
>   
>   	/*
> @@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl
>   		deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev);
>   
>   	param->data_size = param->data_start + needed;
> +
> +out:
> +	up_read(&table->devices_lock);
>   }
>   
>   static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size)
> Index: linux-2.6/drivers/md/dm-table.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-table.c
> +++ linux-2.6/drivers/md/dm-table.c
> @@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re
>   		return -ENOMEM;
>   
>   	INIT_LIST_HEAD(&t->devices);
> +	init_rwsem(&t->devices_lock);
>   
>   	if (!num_targets)
>   		num_targets = KEYS_PER_NODE;
> @@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target
>   	if (dev == disk_devt(t->md->disk))
>   		return -EINVAL;
>   
> +	down_write(&t->devices_lock);
> +
>   	dd = find_device(&t->devices, dev);
>   	if (!dd) {
>   		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> -		if (!dd)
> -			return -ENOMEM;
> +		if (!dd) {
> +			r = -ENOMEM;
> +			goto unlock_ret_r;
> +		}
>   
>   		r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev);
>   		if (r) {
>   			kfree(dd);
> -			return r;
> +			goto unlock_ret_r;
>   		}
>   
>   		refcount_set(&dd->count, 1);
> @@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target
>   	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>   		r = upgrade_mode(dd, mode, t->md);
>   		if (r)
> -			return r;
> +			goto unlock_ret_r;
>   	}
>   	refcount_inc(&dd->count);
>   out:
> +	up_write(&t->devices_lock);
>   	*result = dd->dm_dev;
>   	return 0;
> +
> +unlock_ret_r:
> +	up_write(&t->devices_lock);
> +	return r;
>   }
>   EXPORT_SYMBOL(dm_get_device);
>   
> @@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d
>   void dm_put_device(struct dm_target *ti, struct dm_dev *d)
>   {
>   	int found = 0;
> -	struct list_head *devices = &ti->table->devices;
> +	struct dm_table *t = ti->table;
> +	struct list_head *devices = &t->devices;
>   	struct dm_dev_internal *dd;
>   
> +	down_write(&t->devices_lock);
> +
>   	list_for_each_entry(dd, devices, list) {
>   		if (dd->dm_dev == d) {
>   			found = 1;
> @@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti,
>   	}
>   	if (!found) {
>   		DMERR("%s: device %s not in table devices list",
> -		      dm_device_name(ti->table->md), d->name);
> -		return;
> +		      dm_device_name(t->md), d->name);
> +		goto unlock_ret;
>   	}
>   	if (refcount_dec_and_test(&dd->count)) {
> -		dm_put_table_device(ti->table->md, d);
> +		dm_put_table_device(t->md, d);
>   		list_del(&dd->list);
>   		kfree(dd);
>   	}
> +
> +unlock_ret:
> +	up_write(&t->devices_lock);
>   }
>   EXPORT_SYMBOL(dm_put_device);
>   
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Mike Snitzer]

[-- Attachment #1.1: Type: text/plain, Size: 7059 bytes --]

Please reply to Mikulas's updated patch with your Reviewed-by: and possibly
Tested-by:

Thanks,
Mike

On Tue, Sep 5, 2023, 10:16 PM Li Lingfeng <lilingfeng3@huawei.com> wrote:

> Hi
>
> Thanks to Mikulas for the patch. I have test the patch and it can fix
> the problem.
> Can this patch be applied to mainline?
>
> Thanks.
>
> 在 2023/8/9 18:44, Mikulas Patocka 写道:
> >
> > On Tue, 8 Aug 2023, Mikulas Patocka wrote:
> >
> >> On Wed, 2 Aug 2023, Mike Snitzer wrote:
> >>
> >>> [Cc'ing Mikulas so he can take a look at this too.]
> >> Hi
> >>
> >> I suggest this patch (but it's only compile-tested, so please run some
> >> testsuite on it).
> >>
> >> Mikulas
> > I self-nack that patch - it doesn't work if there are multiple targets
> > calling dm_table_set_devices_mutex in the same table. This is not an
> issue
> > for multipath, but it will be a problem if other targets use this
> > functionality.
> >
> > Here I'm sending a better patch that doesn't need any modification to the
> > targets at all.
> >
> > Mikulas
> >
> >
> >
> > From: Mikulas Patocka <mpatocka at redhat.com>
> > Subject: [PATCH] dm: fix a race condition in retrieve_deps
> >
> > There's a race condition in the multipath target when retrieve_deps races
> > with multipath_message calling dm_get_device and dm_put_device.
> > retrieve_deps walks the list of open devices without holding any lock but
> > multipath may add or remove devices to the list while it is running. The
> > end result may be memory corruption or use-after-free memory access.
> >
> > Fix this bug by introducing a new rw semaphore "devices_lock". We grab
> > devices_lock for read in retrieve_deps and we grab it for write in
> > dm_get_device and dm_put_device.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org
> >
> > ---
> >   drivers/md/dm-core.h  |    1 +
> >   drivers/md/dm-ioctl.c |    7 ++++++-
> >   drivers/md/dm-table.c |   32 ++++++++++++++++++++++++--------
> >   3 files changed, 31 insertions(+), 9 deletions(-)
> >
> > Index: linux-2.6/drivers/md/dm-core.h
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm-core.h
> > +++ linux-2.6/drivers/md/dm-core.h
> > @@ -214,6 +214,7 @@ struct dm_table {
> >
> >       /* a list of devices used by this table */
> >       struct list_head devices;
> > +     struct rw_semaphore devices_lock;
> >
> >       /* events get handed up using this callback */
> >       void (*event_fn)(void *data);
> > Index: linux-2.6/drivers/md/dm-ioctl.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm-ioctl.c
> > +++ linux-2.6/drivers/md/dm-ioctl.c
> > @@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl
> >       struct dm_dev_internal *dd;
> >       struct dm_target_deps *deps;
> >
> > +     down_read(&table->devices_lock);
> > +
> >       deps = get_result_buffer(param, param_size, &len);
> >
> >       /*
> > @@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl
> >       needed = struct_size(deps, dev, count);
> >       if (len < needed) {
> >               param->flags |= DM_BUFFER_FULL_FLAG;
> > -             return;
> > +             goto out;
> >       }
> >
> >       /*
> > @@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl
> >               deps->dev[count++] =
> huge_encode_dev(dd->dm_dev->bdev->bd_dev);
> >
> >       param->data_size = param->data_start + needed;
> > +
> > +out:
> > +     up_read(&table->devices_lock);
> >   }
> >
> >   static int table_deps(struct file *filp, struct dm_ioctl *param,
> size_t param_size)
> > Index: linux-2.6/drivers/md/dm-table.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm-table.c
> > +++ linux-2.6/drivers/md/dm-table.c
> > @@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re
> >               return -ENOMEM;
> >
> >       INIT_LIST_HEAD(&t->devices);
> > +     init_rwsem(&t->devices_lock);
> >
> >       if (!num_targets)
> >               num_targets = KEYS_PER_NODE;
> > @@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target
> >       if (dev == disk_devt(t->md->disk))
> >               return -EINVAL;
> >
> > +     down_write(&t->devices_lock);
> > +
> >       dd = find_device(&t->devices, dev);
> >       if (!dd) {
> >               dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > -             if (!dd)
> > -                     return -ENOMEM;
> > +             if (!dd) {
> > +                     r = -ENOMEM;
> > +                     goto unlock_ret_r;
> > +             }
> >
> >               r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev);
> >               if (r) {
> >                       kfree(dd);
> > -                     return r;
> > +                     goto unlock_ret_r;
> >               }
> >
> >               refcount_set(&dd->count, 1);
> > @@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target
> >       } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> >               r = upgrade_mode(dd, mode, t->md);
> >               if (r)
> > -                     return r;
> > +                     goto unlock_ret_r;
> >       }
> >       refcount_inc(&dd->count);
> >   out:
> > +     up_write(&t->devices_lock);
> >       *result = dd->dm_dev;
> >       return 0;
> > +
> > +unlock_ret_r:
> > +     up_write(&t->devices_lock);
> > +     return r;
> >   }
> >   EXPORT_SYMBOL(dm_get_device);
> >
> > @@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d
> >   void dm_put_device(struct dm_target *ti, struct dm_dev *d)
> >   {
> >       int found = 0;
> > -     struct list_head *devices = &ti->table->devices;
> > +     struct dm_table *t = ti->table;
> > +     struct list_head *devices = &t->devices;
> >       struct dm_dev_internal *dd;
> >
> > +     down_write(&t->devices_lock);
> > +
> >       list_for_each_entry(dd, devices, list) {
> >               if (dd->dm_dev == d) {
> >                       found = 1;
> > @@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti,
> >       }
> >       if (!found) {
> >               DMERR("%s: device %s not in table devices list",
> > -                   dm_device_name(ti->table->md), d->name);
> > -             return;
> > +                   dm_device_name(t->md), d->name);
> > +             goto unlock_ret;
> >       }
> >       if (refcount_dec_and_test(&dd->count)) {
> > -             dm_put_table_device(ti->table->md, d);
> > +             dm_put_table_device(t->md, d);
> >               list_del(&dd->list);
> >               kfree(dd);
> >       }
> > +
> > +unlock_ret:
> > +     up_write(&t->devices_lock);
> >   }
> >   EXPORT_SYMBOL(dm_put_device);
> >
> >
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel
>

[-- Attachment #1.2: Type: text/html, Size: 9600 bytes --]

[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [dm-devel resend] dm mpath: fix UAF in multipath_message()

[Li Lingfeng]

在 2023/8/9 18:44, Mikulas Patocka 写道:
>
> On Tue, 8 Aug 2023, Mikulas Patocka wrote:
>
>> On Wed, 2 Aug 2023, Mike Snitzer wrote:
>>
>>> [Cc'ing Mikulas so he can take a look at this too.]
>> Hi
>>
>> I suggest this patch (but it's only compile-tested, so please run some
>> testsuite on it).
>>
>> Mikulas
> I self-nack that patch - it doesn't work if there are multiple targets
> calling dm_table_set_devices_mutex in the same table. This is not an issue
> for multipath, but it will be a problem if other targets use this
> functionality.
>
> Here I'm sending a better patch that doesn't need any modification to the
> targets at all.
>
> Mikulas
>
>
>
> From: Mikulas Patocka <mpatocka at redhat.com>
> Subject: [PATCH] dm: fix a race condition in retrieve_deps
>
> There's a race condition in the multipath target when retrieve_deps races
> with multipath_message calling dm_get_device and dm_put_device.
> retrieve_deps walks the list of open devices without holding any lock but
> multipath may add or remove devices to the list while it is running. The
> end result may be memory corruption or use-after-free memory access.
>
> Fix this bug by introducing a new rw semaphore "devices_lock". We grab
> devices_lock for read in retrieve_deps and we grab it for write in
> dm_get_device and dm_put_device.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
>
> ---
>   drivers/md/dm-core.h  |    1 +
>   drivers/md/dm-ioctl.c |    7 ++++++-
>   drivers/md/dm-table.c |   32 ++++++++++++++++++++++++--------
>   3 files changed, 31 insertions(+), 9 deletions(-)
>
> Index: linux-2.6/drivers/md/dm-core.h
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-core.h
> +++ linux-2.6/drivers/md/dm-core.h
> @@ -214,6 +214,7 @@ struct dm_table {
>   
>   	/* a list of devices used by this table */
>   	struct list_head devices;
> +	struct rw_semaphore devices_lock;
>   
>   	/* events get handed up using this callback */
>   	void (*event_fn)(void *data);
> Index: linux-2.6/drivers/md/dm-ioctl.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-ioctl.c
> +++ linux-2.6/drivers/md/dm-ioctl.c
> @@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl
>   	struct dm_dev_internal *dd;
>   	struct dm_target_deps *deps;
>   
> +	down_read(&table->devices_lock);
> +
>   	deps = get_result_buffer(param, param_size, &len);
>   
>   	/*
> @@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl
>   	needed = struct_size(deps, dev, count);
>   	if (len < needed) {
>   		param->flags |= DM_BUFFER_FULL_FLAG;
> -		return;
> +		goto out;
>   	}
>   
>   	/*
> @@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl
>   		deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev);
>   
>   	param->data_size = param->data_start + needed;
> +
> +out:
> +	up_read(&table->devices_lock);
>   }
>   
>   static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size)
> Index: linux-2.6/drivers/md/dm-table.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-table.c
> +++ linux-2.6/drivers/md/dm-table.c
> @@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re
>   		return -ENOMEM;
>   
>   	INIT_LIST_HEAD(&t->devices);
> +	init_rwsem(&t->devices_lock);
>   
>   	if (!num_targets)
>   		num_targets = KEYS_PER_NODE;
> @@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target
>   	if (dev == disk_devt(t->md->disk))
>   		return -EINVAL;
>   
> +	down_write(&t->devices_lock);
> +
>   	dd = find_device(&t->devices, dev);
>   	if (!dd) {
>   		dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> -		if (!dd)
> -			return -ENOMEM;
> +		if (!dd) {
> +			r = -ENOMEM;
> +			goto unlock_ret_r;
> +		}
>   
>   		r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev);
>   		if (r) {
>   			kfree(dd);
> -			return r;
> +			goto unlock_ret_r;
>   		}
>   
>   		refcount_set(&dd->count, 1);
> @@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target
>   	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
>   		r = upgrade_mode(dd, mode, t->md);
>   		if (r)
> -			return r;
> +			goto unlock_ret_r;
>   	}
>   	refcount_inc(&dd->count);
>   out:
> +	up_write(&t->devices_lock);
>   	*result = dd->dm_dev;
>   	return 0;
> +
> +unlock_ret_r:
> +	up_write(&t->devices_lock);
> +	return r;
>   }
>   EXPORT_SYMBOL(dm_get_device);
>   
> @@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d
>   void dm_put_device(struct dm_target *ti, struct dm_dev *d)
>   {
>   	int found = 0;
> -	struct list_head *devices = &ti->table->devices;
> +	struct dm_table *t = ti->table;
> +	struct list_head *devices = &t->devices;
>   	struct dm_dev_internal *dd;
>   
> +	down_write(&t->devices_lock);
> +
>   	list_for_each_entry(dd, devices, list) {
>   		if (dd->dm_dev == d) {
>   			found = 1;
> @@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti,
>   	}
>   	if (!found) {
>   		DMERR("%s: device %s not in table devices list",
> -		      dm_device_name(ti->table->md), d->name);
> -		return;
> +		      dm_device_name(t->md), d->name);
> +		goto unlock_ret;
>   	}
>   	if (refcount_dec_and_test(&dd->count)) {
> -		dm_put_table_device(ti->table->md, d);
> +		dm_put_table_device(t->md, d);
>   		list_del(&dd->list);
>   		kfree(dd);
>   	}
> +
> +unlock_ret:
> +	up_write(&t->devices_lock);
>   }
>   EXPORT_SYMBOL(dm_put_device);
>   
Tested-by: Li Lingfeng <lilingfeng3@huawei.com>
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel