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

2023-08-02 Thread Mike Snitzer
On Thu, May 18 2023 at  8:11P -0400,
lilingfeng (A)  wrote:

> 
> 在 2022/10/18 3:56, Mike Snitzer 写道:
> > On Mon, Oct 10 2022 at 10:41P -0400,
> > Luo Meng  wrote:
> > 
> > > From: Luo Meng 
> > > 
> > > 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(>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 
> > > ---
> > >   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), );
> > > + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), , 
> > > 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(>devices, dev);
> > >   if (!dd) {
> > > - dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > - if (!dd)
> > > - return -ENOMEM;
> > > -
> > > - if ((r = dm_get_table_device(t->md, dev, mode, >dm_dev))) {
> > > - kfree(dd);
> > > - return r;
> > > - }
> > > + if (create_dd) {
> > > + dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > + if (!dd)
> > > + return -ENOMEM;
> > > - refcount_set(>count, 1);
> > > - list_add(>list, >devices);
> > > - goto out;
> > > + if ((r = dm_get_table_device(t->md, dev, mode, 
> > > >dm_dev))) {
> > > + kfree(dd);
> > > + return r;
> > > + }
> > > + refcount_set(>count, 1);
> > > + list_add(>list, >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,
> > >  

Re: [dm-devel] [PATCH v13 3/9] block: add emulation for copy

2023-08-02 Thread Kent Overstreet
On Tue, Aug 01, 2023 at 06:37:02PM +0530, Nitesh Shetty wrote:
> On 23/07/20 09:50AM, Christoph Hellwig wrote:
> > > +static void *blkdev_copy_alloc_buf(sector_t req_size, sector_t 
> > > *alloc_size,
> > > + gfp_t gfp_mask)
> > > +{
> > > + int min_size = PAGE_SIZE;
> > > + void *buf;
> > > +
> > > + while (req_size >= min_size) {
> > > + buf = kvmalloc(req_size, gfp_mask);
> > > + if (buf) {
> > > + *alloc_size = req_size;
> > > + return buf;
> > > + }
> > > + /* retry half the requested size */
> > > + req_size >>= 1;
> > > + }
> > > +
> > > + return NULL;
> > 
> > Is there any good reason for using vmalloc instead of a bunch
> > of distcontiguous pages?
> > 
> 
> kvmalloc seemed convenient for the purpose. We will need to call alloc_page
> in a loop to guarantee discontigous pages. Do you prefer that over kvmalloc?

No, kvmalloc should be the preferred approach here now: with large
folios, we're now getting better about doing more large memory
allocations and avoiding fragmentation, so in practice this won't be a
vmalloc allocation except in exceptional circumstances, and performance
will be better and the code will be simpler doing a single large
allocation.

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