[3/4] dm: convert dm_dev_internal.count from atomic_t to refcount_t
diff mbox

Message ID 2236FBA76BA1254E88B949DDB74E612B802C3EDA@IRSMSX102.ger.corp.intel.com
State Superseded, archived
Delegated to: Mike Snitzer
Headers show

Commit Message

Elena Reshetova Nov. 28, 2017, 10:07 a.m. UTC
> 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:

should initialize refcounters to 1 always and do increments from there if needed. 
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!

Best Regards,
Elena.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Mike Snitzer Nov. 28, 2017, 7:02 p.m. UTC | #1
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
Elena Reshetova Nov. 29, 2017, 8:05 a.m. UTC | #2
> 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

Patch
diff mbox

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