Message-ID: <2236FBA76BA1254E88B949DDB74E612B802C4743@IRSMSX102.ger.corp.intel.com>
Date:   Wed, 29 Nov 2017 08:05:17 +0000
From:   "Reshetova, Elena" <elena.reshetova@...el.com>
To:     Mike Snitzer <snitzer@...hat.com>
CC:     Alasdair G Kergon <agk@...hat.com>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "shli@...nel.org" <shli@...nel.org>,
        "koverstreet@...gle.com" <koverstreet@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-raid@...r.kernel.org" <linux-raid@...r.kernel.org>,
        "linux-bcache@...r.kernel.org" <linux-bcache@...r.kernel.org>,
        "ejt@...hat.com" <ejt@...hat.com>,
        "kent.overstreet@...il.com" <kent.overstreet@...il.com>,
        Zdenek Kabelac <zkabelac@...hat.com>
Subject: RE: [PATCH 3/4] dm: convert dm_dev_internal.count from atomic_t to
 refcount_t

> On Tue, Nov 28 2017 at  5:07am -0500,
> Reshetova, Elena <elena.reshetova@...el.com> wrote:
> 
> >
> > > On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena
> > > <elena.reshetova@...el.com> wrote:
> > > >> On Fri, Oct 20, 2017 at 10:37:38AM +0300, Elena Reshetova wrote:
> > > >> >     } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> > > >> >             r = upgrade_mode(dd, mode, t->md);
> > > >> >             if (r)
> > > >> >                     return r;
> > > >> > +           refcount_inc(&dd->count);
> > > >> >     }
> > > >>
> > > >> Missing here:
> > > >>
> > > >>         else
> > > >>               refcount_inc(&dd->count);
> > > >>
> > > >> ?
> > > >
> > > > Oh, yes, thanks for catching this! I think this got unnoticed so far and patch
> was
> > > merged, so I am going to send a followup patch now.
> > >
> > > I pushed this fix and will send to Linus next week:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-
> > > dm.git/commit/?h=dm-
> 4.15&id=d908af82d06cc420f9581c97c6db941cb87e4434
> >
> >
> > I guess you mean this commit:
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-
> dm.git/commit/?h=for-next&id=c2318d07ead871f058dda62e942ed7b6b1c1cfcf
> >
> > Unfortunately it is not correct:
> >
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index 88130b5..f6d32ee 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -451,15 +451,15 @@ int dm_get_device(struct dm_target *ti, const char
> *path, fmode_t mode,
> >  			return r;
> >  		}
> >
> > -		refcount_set(&dd->count, 1);
> > +		refcount_set(&dd->count, 0);
> >  		list_add(&dd->list, &t->devices);
> >
> >  	} else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> >  		r = upgrade_mode(dd, mode, t->md);
> >  		if (r)
> >  			return r;
> > -		refcount_inc(&dd->count);
> >  	}
> > +	refcount_inc(&dd->count);
> >
> > Problem will be here if you hit this refcount_inc() after the refcount_set(&dd-
> >count, 0) earlier.
> > refcount_inc() does not increment on zero value *ever* for security reasons
> and instead people
> > should initialize refcounters to 1 always and do increments from there if
> needed.
> 
> include/linux/refcount.h:refcount_inc() definitely doesn't avoid
> incrementing zero value.

Ok, to be fully precise there are 3 different cases (depending on config options):

1) refcount_t = atomic_t and in this case yes, nothing prevents increment, but no
 protection is given, so we hope such cases are disabled for any distros that care about
security

2) CONFIG_FULL_REFCOUNT is on and refcount_t uses arch. independent implementation
In lib/refcount.c. In this case refcount_inc() won't increment from zero. It is really more
than just a WARN(), increment fails inside refcount_inc_not_zero() used underneath. 

3) arch. dependent implementation is used for refcount_t. Here different options are
possible based on how arch. decides to implement this. Currently we only have x86
one (arch/x86/include/asm/refcount.h) and it is indeed allows increments from zero to happen. 

So, what I described above was the worst case, but since we need the code to work reliably
in each case, we have to take it into account. 


> 
> Neither does lib/refcount.c:refcount_inc() but it does spew a WARN_ON by
> assuming a zero value means use-after-free.

No, it does not increment really, it is not just a WARN(). 
https://elixir.free-electrons.com/linux/latest/source/lib/refcount.c#L151
It uses refcount_inc_not_zero() underneath. 


> 
> > This was the reason for the initial change I did, my mistake was just to forget to
> increment it also
> > in case condition (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) fails.
> >
> > I have issues with my intel smpt server for sending patches (I will get it fixed
> tomorrow from internal network),
> > so I am attaching the patch I did end of last week to this thread instead (or
> alternatively can properly send it tomorrow after fix).
> > Sorry for the delay!
> 
> I was tempted to revert your original commits that switch DM code to
> using refcount_t.  Already proved more trouble than it is worth.
> 
> But I'll drop my commit and take your fix.

Thank you very much and sorry for the troubles!
Unfortunately none of us is free of mistakes, good that this one was caught so fast!

When it comes to value, it does provide security value for your code and makes
sure that your reference counters would not be a new target of many similar CVEs
we had in past around this. Overall each conversion matters since there is less and less
potential holes attackers can try to squeeze themselves!

Best Regards,
Elena.


> 
> Mike

Powered by blists - more mailing lists