| Message ID | 2236FBA76BA1254E88B949DDB74E612B802C3EDA@IRSMSX102.ger.corp.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Tue, Nov 28 2017 at 5:07am -0500, Reshetova, Elena <elena.reshetova@intel.com> wrote: > > > On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena > > <elena.reshetova@intel.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. Neither does lib/refcount.c:refcount_inc() but it does spew a WARN_ON by assuming a zero value means use-after-free. > 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. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
> On Tue, Nov 28 2017 at 5:07am -0500, > Reshetova, Elena <elena.reshetova@intel.com> wrote: > > > > > > On Fri, Nov 24, 2017 at 2:36 AM, Reshetova, Elena > > > <elena.reshetova@intel.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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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