* [PATCH 0/3] v2 ipc subsystem refcount coversions @ 2017-07-07 8:59 Elena Reshetova 2017-07-07 8:59 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Elena Reshetova @ 2017-07-07 8:59 UTC (permalink / raw) To: linux-kernel Cc: peterz, gregkh, akpm, ebiederm, mingo, adobriyan, serge, arozansk, dave, keescook, Elena Reshetova Changes in v2: * add a new patch on kern_ipc_perm.refcount * drop the patch on ipc_rcu.refcount * rebase on top of linux-next/master * Now by default refcount_t = atomic_t (*) and uses all atomic standard operations unless CONFIG_REFCOUNT_FULL is enabled. This is a compromise for the systems that are critical on performance (such as net) and cannot accept even slight delay on the refcounter operations. This series, for ipc subsystem components, replaces atomic_t reference counters with the new refcount_t type and API (see include/linux/refcount.h). By doing this we prevent intentional or accidental underflows or overflows that can led to use-after-free vulnerabilities. The patches are fully independent and can be cherry-picked separately. If there are no objections to the patches, please merge them via respective trees. * The respective change is currently merged into -next as "locking/refcount: Create unchecked atomic_t implementation". Elena Reshetova (3): ipc: convert ipc_namespace.count from atomic_t to refcount_t ipc: convert sem_undo_list.refcnt from atomic_t to refcount_t ipc: convert kern_ipc_perm.refcount from atomic_t to refcount_t include/linux/ipc.h | 3 ++- include/linux/ipc_namespace.h | 5 +++-- ipc/msgutil.c | 2 +- ipc/namespace.c | 4 ++-- ipc/sem.c | 8 ++++---- ipc/util.c | 6 +++--- 6 files changed, 15 insertions(+), 13 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-07 8:59 [PATCH 0/3] v2 ipc subsystem refcount coversions Elena Reshetova @ 2017-07-07 8:59 ` Elena Reshetova 2017-07-09 21:59 ` Eric W. Biederman 2017-07-07 8:59 ` [PATCH 2/3] ipc: convert sem_undo_list.refcnt " Elena Reshetova 2017-07-07 8:59 ` [PATCH 3/3] ipc: convert kern_ipc_perm.refcount " Elena Reshetova 2 siblings, 1 reply; 26+ messages in thread From: Elena Reshetova @ 2017-07-07 8:59 UTC (permalink / raw) To: linux-kernel Cc: peterz, gregkh, akpm, ebiederm, mingo, adobriyan, serge, arozansk, dave, keescook, Elena Reshetova, Hans Liljestrand, David Windsor refcount_t type and corresponding API should be used instead of atomic_t when the variable is used as a reference counter. This allows to avoid accidental refcounter overflows that might lead to use-after-free situations. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: David Windsor <dwindsor@gmail.com> --- include/linux/ipc_namespace.h | 5 +++-- ipc/msgutil.c | 2 +- ipc/namespace.c | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h index 65327ee..e81445c 100644 --- a/include/linux/ipc_namespace.h +++ b/include/linux/ipc_namespace.h @@ -7,6 +7,7 @@ #include <linux/notifier.h> #include <linux/nsproxy.h> #include <linux/ns_common.h> +#include <linux/refcount.h> struct user_namespace; @@ -19,7 +20,7 @@ struct ipc_ids { }; struct ipc_namespace { - atomic_t count; + refcount_t count; struct ipc_ids ids[3]; int sem_ctls[4]; @@ -118,7 +119,7 @@ extern struct ipc_namespace *copy_ipcs(unsigned long flags, static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns) { if (ns) - atomic_inc(&ns->count); + refcount_inc(&ns->count); return ns; } diff --git a/ipc/msgutil.c b/ipc/msgutil.c index bf74eaa..8459802 100644 --- a/ipc/msgutil.c +++ b/ipc/msgutil.c @@ -29,7 +29,7 @@ DEFINE_SPINLOCK(mq_lock); * and not CONFIG_IPC_NS. */ struct ipc_namespace init_ipc_ns = { - .count = ATOMIC_INIT(1), + .count = REFCOUNT_INIT(1), .user_ns = &init_user_ns, .ns.inum = PROC_IPC_INIT_INO, #ifdef CONFIG_IPC_NS diff --git a/ipc/namespace.c b/ipc/namespace.c index b4d80f9..7af6e6b 100644 --- a/ipc/namespace.c +++ b/ipc/namespace.c @@ -50,7 +50,7 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns, goto fail_free; ns->ns.ops = &ipcns_operations; - atomic_set(&ns->count, 1); + refcount_set(&ns->count, 1); ns->user_ns = get_user_ns(user_ns); ns->ucounts = ucounts; @@ -144,7 +144,7 @@ static void free_ipc_ns(struct ipc_namespace *ns) */ void put_ipc_ns(struct ipc_namespace *ns) { - if (atomic_dec_and_lock(&ns->count, &mq_lock)) { + if (refcount_dec_and_lock(&ns->count, &mq_lock)) { mq_clear_sbinfo(ns); spin_unlock(&mq_lock); mq_put_mnt(ns); -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-07 8:59 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova @ 2017-07-09 21:59 ` Eric W. Biederman 2017-07-10 6:48 ` Reshetova, Elena 2017-07-19 22:35 ` Andrew Morton 0 siblings, 2 replies; 26+ messages in thread From: Eric W. Biederman @ 2017-07-09 21:59 UTC (permalink / raw) To: Elena Reshetova Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor Elena Reshetova <elena.reshetova@intel.com> writes: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. In this patch you can see all of the uses of the count. What accidental refcount overflows are possible? Eric > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > Signed-off-by: David Windsor <dwindsor@gmail.com> > --- > include/linux/ipc_namespace.h | 5 +++-- > ipc/msgutil.c | 2 +- > ipc/namespace.c | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h > index 65327ee..e81445c 100644 > --- a/include/linux/ipc_namespace.h > +++ b/include/linux/ipc_namespace.h > @@ -7,6 +7,7 @@ > #include <linux/notifier.h> > #include <linux/nsproxy.h> > #include <linux/ns_common.h> > +#include <linux/refcount.h> > > struct user_namespace; > > @@ -19,7 +20,7 @@ struct ipc_ids { > }; > > struct ipc_namespace { > - atomic_t count; > + refcount_t count; > struct ipc_ids ids[3]; > > int sem_ctls[4]; > @@ -118,7 +119,7 @@ extern struct ipc_namespace *copy_ipcs(unsigned long flags, > static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns) > { > if (ns) > - atomic_inc(&ns->count); > + refcount_inc(&ns->count); > return ns; > } > > diff --git a/ipc/msgutil.c b/ipc/msgutil.c > index bf74eaa..8459802 100644 > --- a/ipc/msgutil.c > +++ b/ipc/msgutil.c > @@ -29,7 +29,7 @@ DEFINE_SPINLOCK(mq_lock); > * and not CONFIG_IPC_NS. > */ > struct ipc_namespace init_ipc_ns = { > - .count = ATOMIC_INIT(1), > + .count = REFCOUNT_INIT(1), > .user_ns = &init_user_ns, > .ns.inum = PROC_IPC_INIT_INO, > #ifdef CONFIG_IPC_NS > diff --git a/ipc/namespace.c b/ipc/namespace.c > index b4d80f9..7af6e6b 100644 > --- a/ipc/namespace.c > +++ b/ipc/namespace.c > @@ -50,7 +50,7 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns, > goto fail_free; > ns->ns.ops = &ipcns_operations; > > - atomic_set(&ns->count, 1); > + refcount_set(&ns->count, 1); > ns->user_ns = get_user_ns(user_ns); > ns->ucounts = ucounts; > > @@ -144,7 +144,7 @@ static void free_ipc_ns(struct ipc_namespace *ns) > */ > void put_ipc_ns(struct ipc_namespace *ns) > { > - if (atomic_dec_and_lock(&ns->count, &mq_lock)) { > + if (refcount_dec_and_lock(&ns->count, &mq_lock)) { > mq_clear_sbinfo(ns); > spin_unlock(&mq_lock); > mq_put_mnt(ns); ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-09 21:59 ` Eric W. Biederman @ 2017-07-10 6:48 ` Reshetova, Elena 2017-07-10 8:37 ` Eric W. Biederman 2017-07-19 22:35 ` Andrew Morton 1 sibling, 1 reply; 26+ messages in thread From: Reshetova, Elena @ 2017-07-10 6:48 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor > Elena Reshetova <elena.reshetova@intel.com> writes: > > > refcount_t type and corresponding API should be > > used instead of atomic_t when the variable is used as > > a reference counter. This allows to avoid accidental > > refcounter overflows that might lead to use-after-free > > situations. > > In this patch you can see all of the uses of the count. > What accidental refcount overflows are possible? Even if one can guarantee and prove that in the current implementation there are no overflows possible, we can't say that for sure for any future implementation. Bugs might always happen unfortunately, but if we convert the refcounter to a safer type we can be sure that overflows are not possible. Does it make sense to you? Best Regards, Elena. > > Eric > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Signed-off-by: David Windsor <dwindsor@gmail.com> > > --- > > include/linux/ipc_namespace.h | 5 +++-- > > ipc/msgutil.c | 2 +- > > ipc/namespace.c | 4 ++-- > > 3 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h > > index 65327ee..e81445c 100644 > > --- a/include/linux/ipc_namespace.h > > +++ b/include/linux/ipc_namespace.h > > @@ -7,6 +7,7 @@ > > #include <linux/notifier.h> > > #include <linux/nsproxy.h> > > #include <linux/ns_common.h> > > +#include <linux/refcount.h> > > > > struct user_namespace; > > > > @@ -19,7 +20,7 @@ struct ipc_ids { > > }; > > > > struct ipc_namespace { > > - atomic_t count; > > + refcount_t count; > > struct ipc_ids ids[3]; > > > > int sem_ctls[4]; > > @@ -118,7 +119,7 @@ extern struct ipc_namespace *copy_ipcs(unsigned long > flags, > > static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns) > > { > > if (ns) > > - atomic_inc(&ns->count); > > + refcount_inc(&ns->count); > > return ns; > > } > > > > diff --git a/ipc/msgutil.c b/ipc/msgutil.c > > index bf74eaa..8459802 100644 > > --- a/ipc/msgutil.c > > +++ b/ipc/msgutil.c > > @@ -29,7 +29,7 @@ DEFINE_SPINLOCK(mq_lock); > > * and not CONFIG_IPC_NS. > > */ > > struct ipc_namespace init_ipc_ns = { > > - .count = ATOMIC_INIT(1), > > + .count = REFCOUNT_INIT(1), > > .user_ns = &init_user_ns, > > .ns.inum = PROC_IPC_INIT_INO, > > #ifdef CONFIG_IPC_NS > > diff --git a/ipc/namespace.c b/ipc/namespace.c > > index b4d80f9..7af6e6b 100644 > > --- a/ipc/namespace.c > > +++ b/ipc/namespace.c > > @@ -50,7 +50,7 @@ static struct ipc_namespace *create_ipc_ns(struct > user_namespace *user_ns, > > goto fail_free; > > ns->ns.ops = &ipcns_operations; > > > > - atomic_set(&ns->count, 1); > > + refcount_set(&ns->count, 1); > > ns->user_ns = get_user_ns(user_ns); > > ns->ucounts = ucounts; > > > > @@ -144,7 +144,7 @@ static void free_ipc_ns(struct ipc_namespace *ns) > > */ > > void put_ipc_ns(struct ipc_namespace *ns) > > { > > - if (atomic_dec_and_lock(&ns->count, &mq_lock)) { > > + if (refcount_dec_and_lock(&ns->count, &mq_lock)) { > > mq_clear_sbinfo(ns); > > spin_unlock(&mq_lock); > > mq_put_mnt(ns); ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 6:48 ` Reshetova, Elena @ 2017-07-10 8:37 ` Eric W. Biederman 2017-07-10 9:34 ` Alexey Dobriyan 2017-07-10 9:56 ` Reshetova, Elena 0 siblings, 2 replies; 26+ messages in thread From: Eric W. Biederman @ 2017-07-10 8:37 UTC (permalink / raw) To: Reshetova, Elena Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor "Reshetova, Elena" <elena.reshetova@intel.com> writes: 2>> Elena Reshetova <elena.reshetova@intel.com> writes: >> >> > refcount_t type and corresponding API should be >> > used instead of atomic_t when the variable is used as >> > a reference counter. This allows to avoid accidental >> > refcounter overflows that might lead to use-after-free >> > situations. >> >> In this patch you can see all of the uses of the count. >> What accidental refcount overflows are possible? > > Even if one can guarantee and prove that in the current implementation > there are no overflows possible, we can't say that for > sure for any future implementation. Bugs might always happen > unfortunately, but if we convert the refcounter to a safer > type we can be sure that overflows are not possible. > > Does it make sense to you? Not for code that is likely to remain unchanged for a decade no. This looks like a large set of unautomated changes without any real thought put into it. That almost always results in a typo somewhere that breaks things. So there is no benefit to the code, and a non-zero chance that there will be a typo breaking the code. All to harden the code for an unlikely future when the code is updated with a full test cycle and people paying attention. Introduce a bug now to avoid a bug in the future. That seems like a very poor engineering trade off. Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 8:37 ` Eric W. Biederman @ 2017-07-10 9:34 ` Alexey Dobriyan 2017-07-10 11:19 ` Eric W. Biederman 2017-07-10 9:56 ` Reshetova, Elena 1 sibling, 1 reply; 26+ messages in thread From: Alexey Dobriyan @ 2017-07-10 9:34 UTC (permalink / raw) To: Eric W. Biederman Cc: Reshetova, Elena, linux-kernel, peterz, gregkh, akpm, mingo, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor On Mon, Jul 10, 2017 at 11:37 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > "Reshetova, Elena" <elena.reshetova@intel.com> writes: > > 2>> Elena Reshetova <elena.reshetova@intel.com> writes: >>> >>> > refcount_t type and corresponding API should be >>> > used instead of atomic_t when the variable is used as >>> > a reference counter. This allows to avoid accidental >>> > refcounter overflows that might lead to use-after-free >>> > situations. >>> >>> In this patch you can see all of the uses of the count. >>> What accidental refcount overflows are possible? >> >> Even if one can guarantee and prove that in the current implementation >> there are no overflows possible, we can't say that for >> sure for any future implementation. Bugs might always happen >> unfortunately, but if we convert the refcounter to a safer >> type we can be sure that overflows are not possible. >> >> Does it make sense to you? > > Not for code that is likely to remain unchanged for a decade no. > > This looks like a large set of unautomated changes without any real > thought put into it. That almost always results in a typo somewhere > that breaks things. This is nonsense. The wrong code would simply emit a warning which are caught very quickly. > So there is no benefit to the code, and a non-zero chance that there > will be a typo breaking the code. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 9:34 ` Alexey Dobriyan @ 2017-07-10 11:19 ` Eric W. Biederman 0 siblings, 0 replies; 26+ messages in thread From: Eric W. Biederman @ 2017-07-10 11:19 UTC (permalink / raw) To: Alexey Dobriyan Cc: Reshetova, Elena, linux-kernel, peterz, gregkh, akpm, mingo, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor Alexey Dobriyan <adobriyan@gmail.com> writes: > On Mon, Jul 10, 2017 at 11:37 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> "Reshetova, Elena" <elena.reshetova@intel.com> writes: >> >> 2>> Elena Reshetova <elena.reshetova@intel.com> writes: >>>> >>>> > refcount_t type and corresponding API should be >>>> > used instead of atomic_t when the variable is used as >>>> > a reference counter. This allows to avoid accidental >>>> > refcounter overflows that might lead to use-after-free >>>> > situations. >>>> >>>> In this patch you can see all of the uses of the count. >>>> What accidental refcount overflows are possible? >>> >>> Even if one can guarantee and prove that in the current implementation >>> there are no overflows possible, we can't say that for >>> sure for any future implementation. Bugs might always happen >>> unfortunately, but if we convert the refcounter to a safer >>> type we can be sure that overflows are not possible. >>> >>> Does it make sense to you? >> >> Not for code that is likely to remain unchanged for a decade no. >> >> This looks like a large set of unautomated changes without any real >> thought put into it. That almost always results in a typo somewhere >> that breaks things. > > This is nonsense. The wrong code would simply emit a warning > which are caught very quickly. That depends on the typo. Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 8:37 ` Eric W. Biederman 2017-07-10 9:34 ` Alexey Dobriyan @ 2017-07-10 9:56 ` Reshetova, Elena 2017-07-10 11:26 ` Eric W. Biederman 1 sibling, 1 reply; 26+ messages in thread From: Reshetova, Elena @ 2017-07-10 9:56 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor > "Reshetova, Elena" <elena.reshetova@intel.com> writes: > > 2>> Elena Reshetova <elena.reshetova@intel.com> writes: > >> > >> > refcount_t type and corresponding API should be > >> > used instead of atomic_t when the variable is used as > >> > a reference counter. This allows to avoid accidental > >> > refcounter overflows that might lead to use-after-free > >> > situations. > >> > >> In this patch you can see all of the uses of the count. > >> What accidental refcount overflows are possible? > > > > Even if one can guarantee and prove that in the current implementation > > there are no overflows possible, we can't say that for > > sure for any future implementation. Bugs might always happen > > unfortunately, but if we convert the refcounter to a safer > > type we can be sure that overflows are not possible. > > > > Does it make sense to you? > > Not for code that is likely to remain unchanged for a decade no. Can we really be sure for any kernel code about this? And does it make sense to trust our security on a fact like this? > > This looks like a large set of unautomated changes without any real > thought put into it. We are soon into the end of the first year that we started to look into refcounter overflow/underflow problem and coming up this far was not easy enough (just check all the millions of emails on kernel-hardening mailing list). Each refcount_t conversion candidate was first found by Coccinelle analysis and then manually checked and converted. The story of refcount_t API and all discussions go even further. So you can't really claim that there is no " thought put into it " :) That almost always results in a typo somewhere > that breaks things. > > So there is no benefit to the code, and a non-zero chance that there > will be a typo breaking the code. The code is very active on issuing WARNs when anything goes wrong. Using this feature we have not only found errors in conversions, but sometimes errors in code itself. So, any bug would be actually much faster visible than using old atomic_t interface. In addition by default refcount_t equals to atomic, which also gives a possibility to make a softer transition and catch all related bugs in couple of cycles when enabling CONFIG_REFCOUNT_FULL. Best Regards, Elena. > > All to harden the code for an unlikely future when the code is > updated with a full test cycle and people paying attention. > > Introduce a bug now to avoid a bug in the future. That seems like a > very poor engineering trade off. > > Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 9:56 ` Reshetova, Elena @ 2017-07-10 11:26 ` Eric W. Biederman 2017-07-10 12:11 ` Reshetova, Elena 0 siblings, 1 reply; 26+ messages in thread From: Eric W. Biederman @ 2017-07-10 11:26 UTC (permalink / raw) To: Reshetova, Elena Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor "Reshetova, Elena" <elena.reshetova@intel.com> writes: >> "Reshetova, Elena" <elena.reshetova@intel.com> writes: >> >> 2>> Elena Reshetova <elena.reshetova@intel.com> writes: >> >> >> >> > refcount_t type and corresponding API should be >> >> > used instead of atomic_t when the variable is used as >> >> > a reference counter. This allows to avoid accidental >> >> > refcounter overflows that might lead to use-after-free >> >> > situations. >> >> >> >> In this patch you can see all of the uses of the count. >> >> What accidental refcount overflows are possible? >> > >> > Even if one can guarantee and prove that in the current implementation >> > there are no overflows possible, we can't say that for >> > sure for any future implementation. Bugs might always happen >> > unfortunately, but if we convert the refcounter to a safer >> > type we can be sure that overflows are not possible. >> > >> > Does it make sense to you? >> >> Not for code that is likely to remain unchanged for a decade no. > > Can we really be sure for any kernel code about this? And does it make > sense to trust our security on a fact like this? But refcount_t doesn't fix anything. At best it changes a bad bug to a less bad bug. So now my machine OOMS instead of allows a memory overwrite. It still doesn't work. Plus refcount_t does not provide any safety on the architectures where it is a noop. >> This looks like a large set of unautomated changes without any real >> thought put into it. > > We are soon into the end of the first year that we started to look into > refcounter overflow/underflow problem and coming up this far was > not easy enough (just check all the millions of emails on kernel-hardening > mailing list). Each refcount_t conversion candidate was first found by Coccinelle > analysis and then manually checked and converted. The story of > refcount_t API and all discussions go even further. > So you can't really claim that there is no " thought put into it " :) But the conversion of the instance happens without thought and manually. Which is a good recipe for typos. Which is what I am saying. There have been lots of conversions like that in the kernel and practically every one has introduced at least one typo. So from an engineering standpoint it is a very valid question to ask about. And I find the apparent insistence that you don't make typos very disturbing. > That almost always results in a typo somewhere >> that breaks things. >> >> So there is no benefit to the code, and a non-zero chance that there >> will be a typo breaking the code. > > The code is very active on issuing WARNs when anything goes wrong. > Using this feature we have not only found errors in conversions, but > sometimes errors in code itself. So, any bug would be actually much > faster visible than using old atomic_t interface. > > In addition by default refcount_t equals to atomic, which also gives a > possibility to make a softer transition and catch all related bugs in couple > of cycles when enabling CONFIG_REFCOUNT_FULL. But if you make a typo and change one operation for another I don't see how any of that applies. And that is what it looks like I we are looking at here. Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 11:26 ` Eric W. Biederman @ 2017-07-10 12:11 ` Reshetova, Elena 2017-07-10 20:32 ` Eric W. Biederman 0 siblings, 1 reply; 26+ messages in thread From: Reshetova, Elena @ 2017-07-10 12:11 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor > "Reshetova, Elena" <elena.reshetova@intel.com> writes: > > >> "Reshetova, Elena" <elena.reshetova@intel.com> writes: > >> > >> 2>> Elena Reshetova <elena.reshetova@intel.com> writes: > >> >> > >> >> > refcount_t type and corresponding API should be > >> >> > used instead of atomic_t when the variable is used as > >> >> > a reference counter. This allows to avoid accidental > >> >> > refcounter overflows that might lead to use-after-free > >> >> > situations. > >> >> > >> >> In this patch you can see all of the uses of the count. > >> >> What accidental refcount overflows are possible? > >> > > >> > Even if one can guarantee and prove that in the current implementation > >> > there are no overflows possible, we can't say that for > >> > sure for any future implementation. Bugs might always happen > >> > unfortunately, but if we convert the refcounter to a safer > >> > type we can be sure that overflows are not possible. > >> > > >> > Does it make sense to you? > >> > >> Not for code that is likely to remain unchanged for a decade no. > > > > Can we really be sure for any kernel code about this? And does it make > > sense to trust our security on a fact like this? > > But refcount_t doesn't fix anything. At best it changes a bad bug to a > less bad bug. So now my machine OOMS instead of allows a memory > overwrite. It still doesn't work. Well, it is a step forward from security standpoint. OOMS is really hard to exploit vs. memory overwrites. Pretty much all exploits need either memory write or memory read, out of memory is really much harder to exploit. > > Plus refcount_t does not provide any safety on the architectures where > it is a noop. Not sure I understood this. What do you mean by "noop"? refcount_t is currently architecture independent. > > >> This looks like a large set of unautomated changes without any real > >> thought put into it. > > > > We are soon into the end of the first year that we started to look into > > refcounter overflow/underflow problem and coming up this far was > > not easy enough (just check all the millions of emails on kernel-hardening > > mailing list). Each refcount_t conversion candidate was first found by Coccinelle > > analysis and then manually checked and converted. The story of > > refcount_t API and all discussions go even further. > > So you can't really claim that there is no " thought put into it " :) > > But the conversion of the instance happens without thought and manually. > Which is a good recipe for typos. Which is what I am saying. > > There have been lots of conversions like that in the kernel and > practically every one has introduced at least one typo. What do you exactly mean by "typo"? Typos should be detected at these stages: 1) typos like wrong function name etc. can be found at compile time (trust me I have found a number of these on the very first iteration with patches) 2) "typos" (not sure if it is correct to call them typos) like usage of wrong refcount_t API vs. original atomic_t API can be found during internal reviews or reviews by maintainers 3) much bigger problem is actually not any typos, but hidden issues that show up only in run-time that detect underflows/overflows or inability to increment from zero. These only are nasty, but given that refcount_t WARNs left and right about them, we can detect them fast. I don't know what is a better recipe for doing API changes like this? Do you have any suggestions? > > So from an engineering standpoint it is a very valid question to ask > about. And I find the apparent insistence that you don't make typos > very disturbing. > > > That almost always results in a typo somewhere > >> that breaks things. > >> > >> So there is no benefit to the code, and a non-zero chance that there > >> will be a typo breaking the code. > > > > The code is very active on issuing WARNs when anything goes wrong. > > Using this feature we have not only found errors in conversions, but > > sometimes errors in code itself. So, any bug would be actually much > > faster visible than using old atomic_t interface. > > > > In addition by default refcount_t equals to atomic, which also gives a > > possibility to make a softer transition and catch all related bugs in couple > > of cycles when enabling CONFIG_REFCOUNT_FULL. > > But if you make a typo and change one operation for another I don't see > how any of that applies. It is hard to make a "typo" to change one operation to another. It is not a one-two char mismatch error. When doing these patches we followed the logic of "less code changes - better" (since less chances of making mistake), so if in some cases functions are changed (like from atomic_sub to refcount_sub_and_test(), or from atomic_inc_not_zero() to atomic_inc() etc.) there was a reason for making it and the change wasn't automatic and without thinking at all. Again, we do have our maintainers also to catch if a change that we did doesn't actually work for them right? Best Regards, Elena. > > And that is what it looks like I we are looking at here. > > Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 12:11 ` Reshetova, Elena @ 2017-07-10 20:32 ` Eric W. Biederman 2017-07-12 9:21 ` Reshetova, Elena 0 siblings, 1 reply; 26+ messages in thread From: Eric W. Biederman @ 2017-07-10 20:32 UTC (permalink / raw) To: Reshetova, Elena Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor "Reshetova, Elena" <elena.reshetova@intel.com> writes: >> "Reshetova, Elena" <elena.reshetova@intel.com> writes: >> >> >> "Reshetova, Elena" <elena.reshetova@intel.com> writes: >> >> >> >> 2>> Elena Reshetova <elena.reshetova@intel.com> writes: >> >> >> >> >> >> > refcount_t type and corresponding API should be >> >> >> > used instead of atomic_t when the variable is used as >> >> >> > a reference counter. This allows to avoid accidental >> >> >> > refcounter overflows that might lead to use-after-free >> >> >> > situations. >> >> >> >> >> >> In this patch you can see all of the uses of the count. >> >> >> What accidental refcount overflows are possible? >> >> > >> >> > Even if one can guarantee and prove that in the current implementation >> >> > there are no overflows possible, we can't say that for >> >> > sure for any future implementation. Bugs might always happen >> >> > unfortunately, but if we convert the refcounter to a safer >> >> > type we can be sure that overflows are not possible. >> >> > >> >> > Does it make sense to you? >> >> >> >> Not for code that is likely to remain unchanged for a decade no. >> > >> > Can we really be sure for any kernel code about this? And does it make >> > sense to trust our security on a fact like this? >> >> But refcount_t doesn't fix anything. At best it changes a bad bug to a >> less bad bug. So now my machine OOMS instead of allows a memory >> overwrite. It still doesn't work. > > Well, it is a step forward from security standpoint. OOMS is really hard > to exploit vs. memory overwrites. Pretty much all exploits need either > memory write or memory read, out of memory is really much harder to > exploit. OOM in production is a denial of service attack. From a serverity point of view an OOM can be considered equivalent to a kernel panic and something that requires a box to reboot. >From a long term perspective I expect we will need to change all reference counters to a type where that is not saturating but instead fails to increment and returns an error. If we want to keep a system functioning in the face of maxing out a reference count that is the only way it can truly be done. >> Plus refcount_t does not provide any safety on the architectures where >> it is a noop. > > Not sure I understood this. What do you mean by "noop"? > refcount_t is currently architecture independent. noop being short for no operation. I believe there were some/most archicture implementations that define refcount_t to atomic_t out of performance concerns. I know I saw patches fly by to that effect. On an architecture where refcount_t is equivalent to atomic_t the change in these patches is a noop. >> >> This looks like a large set of unautomated changes without any real >> >> thought put into it. >> > >> > We are soon into the end of the first year that we started to look into >> > refcounter overflow/underflow problem and coming up this far was >> > not easy enough (just check all the millions of emails on kernel-hardening >> > mailing list). Each refcount_t conversion candidate was first found by Coccinelle >> > analysis and then manually checked and converted. The story of >> > refcount_t API and all discussions go even further. >> > So you can't really claim that there is no " thought put into it " :) >> >> But the conversion of the instance happens without thought and manually. >> Which is a good recipe for typos. Which is what I am saying. >> >> There have been lots of conversions like that in the kernel and >> practically every one has introduced at least one typo. > > What do you exactly mean by "typo"? Typos should be detected at these > stages: An unintentional mistake. The term "thinko" might be more what I am thinking of. Many human mistakes are not slips of the fingers but accidentally giving the wrong command at some level. > 1) typos like wrong function name etc. can be found at compile time > (trust me I have found a number of these on the very first iteration with patches) > 2) "typos" (not sure if it is correct to call them typos) like usage of wrong refcount_t > API vs. original atomic_t API can be found during internal reviews or reviews by maintainers This I worry about most as the mental distance from xxx_inc to xxx_dec can be very short. > 3) much bigger problem is actually not any typos, but hidden issues that show up only > in run-time that detect underflows/overflows or inability to increment from zero. > These only are nasty, but given that refcount_t WARNs left and right about them, > we can detect them fast. Which means I worry about those less. > I don't know what is a better recipe for doing API changes like this? > Do you have any suggestions? I would think a semantic patch targeting a specific lock would be less error prone. I would think that the same semantic patch could be used from lock to lock with just a change of the lock that is being targeted. I strongly suspect that would reduce the chance of accident when dealing with a particular API and being scripted would increase the confidence in the changes. >> So from an engineering standpoint it is a very valid question to ask >> about. And I find the apparent insistence that you don't make typos >> very disturbing. >> >> > That almost always results in a typo somewhere >> >> that breaks things. >> >> >> >> So there is no benefit to the code, and a non-zero chance that there >> >> will be a typo breaking the code. >> > >> > The code is very active on issuing WARNs when anything goes wrong. >> > Using this feature we have not only found errors in conversions, but >> > sometimes errors in code itself. So, any bug would be actually much >> > faster visible than using old atomic_t interface. >> > >> > In addition by default refcount_t equals to atomic, which also gives a >> > possibility to make a softer transition and catch all related bugs in couple >> > of cycles when enabling CONFIG_REFCOUNT_FULL. >> >> But if you make a typo and change one operation for another I don't see >> how any of that applies. > > It is hard to make a "typo" to change one operation to another. It is not a > one-two char mismatch error. Those might be more properly "thinkos" but they do happen. > When doing these patches we followed the > logic of "less code changes - better" (since less chances of making mistake), > so if in some cases functions are changed (like from atomic_sub to > refcount_sub_and_test(), or from atomic_inc_not_zero() to atomic_inc() etc.) > there was a reason for making it and the change wasn't automatic and without > thinking at all. Again, we do have our maintainers also to catch if a change that > we did doesn't actually work for them right? In general a maintainers job is make certain the appropriate code review and due dilligence has happened not nececessarily to perform that code review themselves. Changing from atomoic_inc_not_zero to a simple refcount_inc really troubles me because it makes it harder to switch to something fully correct. Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-10 20:32 ` Eric W. Biederman @ 2017-07-12 9:21 ` Reshetova, Elena 0 siblings, 0 replies; 26+ messages in thread From: Reshetova, Elena @ 2017-07-12 9:21 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel, peterz, gregkh, akpm, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor > "Reshetova, Elena" <elena.reshetova@intel.com> writes: > > >> "Reshetova, Elena" <elena.reshetova@intel.com> writes: > >> > >> >> "Reshetova, Elena" <elena.reshetova@intel.com> writes: > >> >> > >> >> 2>> Elena Reshetova <elena.reshetova@intel.com> writes: > >> >> >> > >> >> >> > refcount_t type and corresponding API should be > >> >> >> > used instead of atomic_t when the variable is used as > >> >> >> > a reference counter. This allows to avoid accidental > >> >> >> > refcounter overflows that might lead to use-after-free > >> >> >> > situations. > >> >> >> > >> >> >> In this patch you can see all of the uses of the count. > >> >> >> What accidental refcount overflows are possible? > >> >> > > >> >> > Even if one can guarantee and prove that in the current implementation > >> >> > there are no overflows possible, we can't say that for > >> >> > sure for any future implementation. Bugs might always happen > >> >> > unfortunately, but if we convert the refcounter to a safer > >> >> > type we can be sure that overflows are not possible. > >> >> > > >> >> > Does it make sense to you? > >> >> > >> >> Not for code that is likely to remain unchanged for a decade no. > >> > > >> > Can we really be sure for any kernel code about this? And does it make > >> > sense to trust our security on a fact like this? > >> > >> But refcount_t doesn't fix anything. At best it changes a bad bug to a > >> less bad bug. So now my machine OOMS instead of allows a memory > >> overwrite. It still doesn't work. > > > > Well, it is a step forward from security standpoint. OOMS is really hard > > to exploit vs. memory overwrites. Pretty much all exploits need either > > memory write or memory read, out of memory is really much harder to > > exploit. > > OOM in production is a denial of service attack. From a serverity point > of view an OOM can be considered equivalent to a kernel panic and > something that requires a box to reboot. Denial of service is usually (unless the system is designed poorly) not the highest security concern. It is one thing to get your system down for a certain period of time vs. telling your customers that all your database of their data and credit cards info is now public in the Internet. So, I would still stand by the statement that it makes the situation better. > > From a long term perspective I expect we will need to change all > reference counters to a type where that is not saturating but instead > fails to increment and returns an error. If we want to keep a system > functioning in the face of maxing out a reference count that is the only > way it can truly be done. I think the work on refcount_t internals will continue. We have a number of things that need to be improved further. Kees has first set of patches in work now, and no one expects it to be the last. However, we also do need to make conversions in the kernel, otherwise we are not moving anywhere and it is also easier to test implications of changes inside refcount_t on as many converted cases as possible when they are merged. > > >> Plus refcount_t does not provide any safety on the architectures where > >> it is a noop. > > > > Not sure I understood this. What do you mean by "noop"? > > refcount_t is currently architecture independent. > > noop being short for no operation. > > I believe there were some/most archicture implementations that define > refcount_t to atomic_t out of performance concerns. I know I saw > patches fly by to that effect. On an architecture where refcount_t is > equivalent to atomic_t the change in these patches is a noop. This choice isn't architecture-dependent now. It is defaults to atomic_t for all architectures unless CONFIG_REFCOUNT_FULL is enabled (then it is arch. independent refcount_t implementation). Next set of patches (still under review) provides x86 arch. dependent fast refcount_t implementation. > > >> >> This looks like a large set of unautomated changes without any real > >> >> thought put into it. > >> > > >> > We are soon into the end of the first year that we started to look into > >> > refcounter overflow/underflow problem and coming up this far was > >> > not easy enough (just check all the millions of emails on kernel-hardening > >> > mailing list). Each refcount_t conversion candidate was first found by > Coccinelle > >> > analysis and then manually checked and converted. The story of > >> > refcount_t API and all discussions go even further. > >> > So you can't really claim that there is no " thought put into it " :) > >> > >> But the conversion of the instance happens without thought and manually. > >> Which is a good recipe for typos. Which is what I am saying. > >> > >> There have been lots of conversions like that in the kernel and > >> practically every one has introduced at least one typo. > > > > What do you exactly mean by "typo"? Typos should be detected at these > > stages: > > An unintentional mistake. The term "thinko" might be more what I am > thinking of. Many human mistakes are not slips of the fingers but > accidentally giving the wrong command at some level. > > > 1) typos like wrong function name etc. can be found at compile time > > (trust me I have found a number of these on the very first iteration with > patches) > > 2) "typos" (not sure if it is correct to call them typos) like usage of wrong > refcount_t > > API vs. original atomic_t API can be found during internal reviews or reviews > by maintainers > > This I worry about most as the mental distance from xxx_inc to xxx_dec > can be very short. I would say the distance is huge :) Can't think of possible way one would make such a mistake. > > > 3) much bigger problem is actually not any typos, but hidden issues that show > up only > > in run-time that detect underflows/overflows or inability to increment from > zero. > > These only are nasty, but given that refcount_t WARNs left and right about > them, > > we can detect them fast. > > Which means I worry about those less. > > > I don't know what is a better recipe for doing API changes like this? > > Do you have any suggestions? > > I would think a semantic patch targeting a specific lock would be less > error prone. I would think that the same semantic patch could be used > from lock to lock with just a change of the lock that is being targeted. I was considering in the beginning of writing a full cocci semantic patch, but based on how spread the code for refcounters might be (take smth like socket's refcounter), how many different edge cases we had, the semantic patch would be so complex (and I would even not really trust the output personally), needs manual verification anyway etc. So, at the end we end up with just using .cocci file to find occurrences, but do conversions manually. And when I say manually, it didn't mean that I manually typed each function name, the process was roughly like this: 1. take a look on the reported occurrence. Determine if it should be converted at the first place 2. If needs conversion, run substitute atomic_* to refcount_* (not this only changes prefixes) 3. resolve cases that need manual addressing (i.e refcount_* analog function doesn't simply exists) 4. check the conversion overall if everything makes sense together, fix other issues (remove unneeded WARNs, change inc to set for initializing refcounting etc.) Note the 4th step has many of these angles. > > I strongly suspect that would reduce the chance of accident when dealing > with a particular API and being scripted would increase the confidence > in the changes. Even if I made a wrong decision with it, I guess now it doesn't matter so much because we have our conversions now and I am very confident that if there are mistakes there, they could not be caught by semantic patch. Unless we write now a script to check the validity of each patch, which is probably doable, but I don't think I have energy for this left. Any volunteers? :) > > >> So from an engineering standpoint it is a very valid question to ask > >> about. And I find the apparent insistence that you don't make typos > >> very disturbing. > >> > >> > That almost always results in a typo somewhere > >> >> that breaks things. > >> >> > >> >> So there is no benefit to the code, and a non-zero chance that there > >> >> will be a typo breaking the code. > >> > > >> > The code is very active on issuing WARNs when anything goes wrong. > >> > Using this feature we have not only found errors in conversions, but > >> > sometimes errors in code itself. So, any bug would be actually much > >> > faster visible than using old atomic_t interface. > >> > > >> > In addition by default refcount_t equals to atomic, which also gives a > >> > possibility to make a softer transition and catch all related bugs in couple > >> > of cycles when enabling CONFIG_REFCOUNT_FULL. > >> > >> But if you make a typo and change one operation for another I don't see > >> how any of that applies. > > > > It is hard to make a "typo" to change one operation to another. It is not a > > one-two char mismatch error. > > Those might be more properly "thinkos" but they do happen. > > > When doing these patches we followed the > > logic of "less code changes - better" (since less chances of making mistake), > > so if in some cases functions are changed (like from atomic_sub to > > refcount_sub_and_test(), or from atomic_inc_not_zero() to atomic_inc() etc.) > > there was a reason for making it and the change wasn't automatic and without > > thinking at all. Again, we do have our maintainers also to catch if a change that > > we did doesn't actually work for them right? > > In general a maintainers job is make certain the appropriate code review > and due dilligence has happened not nececessarily to perform that code > review themselves. > > Changing from atomoic_inc_not_zero to a simple refcount_inc really > troubles me because it makes it harder to switch to something fully > correct. I actually meant case like this (just came up recently, so stayed in my head): static void get_pi_state(struct futex_pi_state *pi_state) { - WARN_ON_ONCE(!atomic_inc_not_zero(&pi_state->refcount)); + refcount_inc(&pi_state->refcount); Just to avoid extra layer of unneeded warning. But even with cases like this as I said we were super conservative and changing as little as possible unless It is 100% clear case or people suggest the change during patch review. Best Regards, Elena. > > Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-09 21:59 ` Eric W. Biederman 2017-07-10 6:48 ` Reshetova, Elena @ 2017-07-19 22:35 ` Andrew Morton 2017-07-19 22:54 ` Davidlohr Bueso 1 sibling, 1 reply; 26+ messages in thread From: Andrew Morton @ 2017-07-19 22:35 UTC (permalink / raw) To: Eric W. Biederman Cc: Elena Reshetova, linux-kernel, peterz, gregkh, mingo, adobriyan, serge, arozansk, dave, keescook, Hans Liljestrand, David Windsor On Sun, 09 Jul 2017 16:59:55 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote: > Elena Reshetova <elena.reshetova@intel.com> writes: > > > refcount_t type and corresponding API should be > > used instead of atomic_t when the variable is used as > > a reference counter. This allows to avoid accidental > > refcounter overflows that might lead to use-after-free > > situations. > > In this patch you can see all of the uses of the count. > What accidental refcount overflows are possible? I do rather dislike these conversions from the point of view of performance overhead and general code bloat. But I seem to have lost that struggle and I don't think any of these are fastpath(?). ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-19 22:35 ` Andrew Morton @ 2017-07-19 22:54 ` Davidlohr Bueso 2017-07-19 22:58 ` Andrew Morton 0 siblings, 1 reply; 26+ messages in thread From: Davidlohr Bueso @ 2017-07-19 22:54 UTC (permalink / raw) To: Andrew Morton Cc: Eric W. Biederman, Elena Reshetova, linux-kernel, peterz, gregkh, mingo, adobriyan, serge, arozansk, keescook, Hans Liljestrand, David Windsor On Wed, 19 Jul 2017, Andrew Morton wrote: >I do rather dislike these conversions from the point of view of >performance overhead and general code bloat. But I seem to have lost >that struggle and I don't think any of these are fastpath(?). Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t implementation), performance is supposed to be ok. It would be lovely to have some actual numbers nonetheless. Thanks, Davidlohr > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-19 22:54 ` Davidlohr Bueso @ 2017-07-19 22:58 ` Andrew Morton 2017-07-19 23:11 ` Davidlohr Bueso 2017-07-20 9:34 ` Ingo Molnar 0 siblings, 2 replies; 26+ messages in thread From: Andrew Morton @ 2017-07-19 22:58 UTC (permalink / raw) To: Davidlohr Bueso Cc: Eric W. Biederman, Elena Reshetova, linux-kernel, peterz, gregkh, mingo, adobriyan, serge, arozansk, keescook, Hans Liljestrand, David Windsor On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > On Wed, 19 Jul 2017, Andrew Morton wrote: > > >I do rather dislike these conversions from the point of view of > >performance overhead and general code bloat. But I seem to have lost > >that struggle and I don't think any of these are fastpath(?). > > Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t > implementation), performance is supposed to be ok. Sure, things are OK for people who disable the feature. But for people who want to enable the feature we really should minimize the cost by avoiding blindly converting sites which simply don't need it: simple, safe, old, well-tested code. Why go and slow down such code? Need to apply some common sense here... > It would be lovely to have > some actual numbers nonetheless. Very much so. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-19 22:58 ` Andrew Morton @ 2017-07-19 23:11 ` Davidlohr Bueso 2017-07-19 23:20 ` Kees Cook 2017-07-20 9:34 ` Ingo Molnar 1 sibling, 1 reply; 26+ messages in thread From: Davidlohr Bueso @ 2017-07-19 23:11 UTC (permalink / raw) To: Andrew Morton Cc: Eric W. Biederman, Elena Reshetova, linux-kernel, peterz, gregkh, mingo, adobriyan, serge, arozansk, keescook, Hans Liljestrand, David Windsor On Wed, 19 Jul 2017, Andrew Morton wrote: >On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > >> On Wed, 19 Jul 2017, Andrew Morton wrote: >> >> >I do rather dislike these conversions from the point of view of >> >performance overhead and general code bloat. But I seem to have lost >> >that struggle and I don't think any of these are fastpath(?). >> >> Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t >> implementation), performance is supposed to be ok. > >Sure, things are OK for people who disable the feature. > >But for people who want to enable the feature we really should minimize >the cost by avoiding blindly converting sites which simply don't need >it: simple, safe, old, well-tested code. Why go and slow down such >code? Need to apply some common sense here... Fair points. > >> It would be lovely to have >> some actual numbers nonetheless. > >Very much so. May I suggest using mmtests with the following config file: https://github.com/gormanm/mmtests/blob/7e070a810bc0af92e592e5121d0ea75fada51aeb/configs/config-global-dhp__workload-ipc-scale-short It will run two of Manfred's ipcscale sem benchmarks. Thanks, Davidlohr ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-19 23:11 ` Davidlohr Bueso @ 2017-07-19 23:20 ` Kees Cook 2017-07-20 0:32 ` Kees Cook 0 siblings, 1 reply; 26+ messages in thread From: Kees Cook @ 2017-07-19 23:20 UTC (permalink / raw) To: Davidlohr Bueso Cc: Andrew Morton, Eric W. Biederman, Elena Reshetova, LKML, Peter Zijlstra, Greg KH, Ingo Molnar, Alexey Dobriyan, Serge E. Hallyn, arozansk, Hans Liljestrand, David Windsor On Wed, Jul 19, 2017 at 4:11 PM, Davidlohr Bueso <dave@stgolabs.net> wrote: > On Wed, 19 Jul 2017, Andrew Morton wrote: > >> On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> >> wrote: >> >>> On Wed, 19 Jul 2017, Andrew Morton wrote: >>> >>> >I do rather dislike these conversions from the point of view of >>> >performance overhead and general code bloat. But I seem to have lost >>> >that struggle and I don't think any of these are fastpath(?). >>> >>> Well, since we now have fd25d19 (locking/refcount: Create unchecked >>> atomic_t >>> implementation), performance is supposed to be ok. >> >> >> Sure, things are OK for people who disable the feature. FWIW, it's off by default. >> >> But for people who want to enable the feature we really should minimize >> the cost by avoiding blindly converting sites which simply don't need >> it: simple, safe, old, well-tested code. Why go and slow down such >> code? Need to apply some common sense here... These are the very code paths we'd want to make sure are well protected since people may never expect them to misbehave when some "small change" goes in. > Fair points. > >>> It would be lovely to have >>> some actual numbers nonetheless. >> >> Very much so. > > May I suggest using mmtests with the following config file: > > https://github.com/gormanm/mmtests/blob/7e070a810bc0af92e592e5121d0ea75fada51aeb/configs/config-global-dhp__workload-ipc-scale-short > > It will run two of Manfred's ipcscale sem benchmarks. I'll see if I can figure out how to use this for testing the fast refcount protection: https://lkml.org/lkml/2017/7/18/1223 Then we could see: before conversion after conversion with CONFIG_REFCOUNT_FULL with fast refcount protection -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-19 23:20 ` Kees Cook @ 2017-07-20 0:32 ` Kees Cook 0 siblings, 0 replies; 26+ messages in thread From: Kees Cook @ 2017-07-20 0:32 UTC (permalink / raw) To: Davidlohr Bueso Cc: Andrew Morton, Eric W. Biederman, Elena Reshetova, LKML, Peter Zijlstra, Greg KH, Ingo Molnar, Alexey Dobriyan, Serge E. Hallyn, arozansk, Hans Liljestrand, David Windsor On Wed, Jul 19, 2017 at 4:20 PM, Kees Cook <keescook@chromium.org> wrote: > On Wed, Jul 19, 2017 at 4:11 PM, Davidlohr Bueso <dave@stgolabs.net> wrote: >> May I suggest using mmtests with the following config file: >> >> https://github.com/gormanm/mmtests/blob/7e070a810bc0af92e592e5121d0ea75fada51aeb/configs/config-global-dhp__workload-ipc-scale-short >> >> It will run two of Manfred's ipcscale sem benchmarks. > > I'll see if I can figure out how to use this for testing the fast > refcount protection: > https://lkml.org/lkml/2017/7/18/1223 > > Then we could see: > > before conversion > after conversion > with CONFIG_REFCOUNT_FULL > with fast refcount protection I have no idea how to read this report. It seems to be mostly noise (multiple baseline runs seem to show greater variability than compared against the other possible results). Test runs were atomic_t, atomic_t-2, refcount_t, refcount-full, and refcount-fast. (Two baselines, refcount_t conversion, with FULL, and with the fast implementation.) Output here: http://pastebin.ubuntu.com/25129382/ -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-19 22:58 ` Andrew Morton 2017-07-19 23:11 ` Davidlohr Bueso @ 2017-07-20 9:34 ` Ingo Molnar 2017-07-20 12:34 ` Eric W. Biederman 1 sibling, 1 reply; 26+ messages in thread From: Ingo Molnar @ 2017-07-20 9:34 UTC (permalink / raw) To: Andrew Morton Cc: Davidlohr Bueso, Eric W. Biederman, Elena Reshetova, linux-kernel, peterz, gregkh, mingo, adobriyan, serge, arozansk, keescook, Hans Liljestrand, David Windsor * Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > > > On Wed, 19 Jul 2017, Andrew Morton wrote: > > > > >I do rather dislike these conversions from the point of view of > > >performance overhead and general code bloat. But I seem to have lost > > >that struggle and I don't think any of these are fastpath(?). > > > > Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t > > implementation), performance is supposed to be ok. > > Sure, things are OK for people who disable the feature. So with the WIP fast-refcount series from Kees: [PATCH v6 0/2] x86: Implement fast refcount overflow protection I believe the robustness difference between optimized-refcount_t and full-refcount_t will be marginal. I.e. we'll be able to have both higher API safety _and_ performance. > But for people who want to enable the feature we really should minimize the cost > by avoiding blindly converting sites which simply don't need it: simple, safe, > old, well-tested code. Why go and slow down such code? Need to apply some > common sense here... It's old, well-tested code _for existing, sane parameters_, until someone finds a decade old bug in one of these with an insane parameters no-one stumbled upon so far, and builds an exploit on top of it. Only by touching all these places do we have a chance to improve things measurably in terms of reducing the probability of bugs. Thanks, Ingo ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-20 9:34 ` Ingo Molnar @ 2017-07-20 12:34 ` Eric W. Biederman 2017-07-20 15:12 ` Kees Cook 0 siblings, 1 reply; 26+ messages in thread From: Eric W. Biederman @ 2017-07-20 12:34 UTC (permalink / raw) To: Ingo Molnar Cc: Andrew Morton, Davidlohr Bueso, Elena Reshetova, linux-kernel, peterz, gregkh, mingo, adobriyan, serge, arozansk, keescook, Hans Liljestrand, David Windsor Ingo Molnar <mingo@kernel.org> writes: > * Andrew Morton <akpm@linux-foundation.org> wrote: > >> On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: >> >> > On Wed, 19 Jul 2017, Andrew Morton wrote: >> > >> > >I do rather dislike these conversions from the point of view of >> > >performance overhead and general code bloat. But I seem to have lost >> > >that struggle and I don't think any of these are fastpath(?). >> > >> > Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t >> > implementation), performance is supposed to be ok. >> >> Sure, things are OK for people who disable the feature. > > So with the WIP fast-refcount series from Kees: > > [PATCH v6 0/2] x86: Implement fast refcount overflow protection > > I believe the robustness difference between optimized-refcount_t and > full-refcount_t will be marginal. > > I.e. we'll be able to have both higher API safety _and_ performance. > >> But for people who want to enable the feature we really should minimize the cost >> by avoiding blindly converting sites which simply don't need it: simple, safe, >> old, well-tested code. Why go and slow down such code? Need to apply some >> common sense here... > > It's old, well-tested code _for existing, sane parameters_, until someone finds a > decade old bug in one of these with an insane parameters no-one stumbled upon so > far, and builds an exploit on top of it. > > Only by touching all these places do we have a chance to improve things measurably > in terms of reducing the probability of bugs. The more I hear people pushing the upsides of refcount_t without considering the downsides the more I dislike it. - refcount_t is really the wrong thing because it uses saturation semantics. So by definition it includes a bug. - refcount_t will only really prevent something if there is an extra increment. That is not the kind of bug people are likely to make. - refcount_t won't help if you have an extra decrement. The bad use-after-free will still happen. - refcount_t won't help if there is a memory stomp. As with an extra decrement the bad use-after-free will still happen. So all I see is a huge amount of code churn to implement a buggy (by definition) refcounting API, that risks adding new bugs and only truly helps with bugs that are unlikely in the first place. I really don't think this is an obvious slam dunk. Eric ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-07-20 12:34 ` Eric W. Biederman @ 2017-07-20 15:12 ` Kees Cook 0 siblings, 0 replies; 26+ messages in thread From: Kees Cook @ 2017-07-20 15:12 UTC (permalink / raw) To: Eric W. Biederman Cc: Ingo Molnar, Andrew Morton, Davidlohr Bueso, Elena Reshetova, LKML, Peter Zijlstra, Greg KH, Ingo Molnar, Alexey Dobriyan, Serge E. Hallyn, arozansk, Hans Liljestrand, David Windsor On Thu, Jul 20, 2017 at 5:34 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Ingo Molnar <mingo@kernel.org> writes: > >> * Andrew Morton <akpm@linux-foundation.org> wrote: >> >>> On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: >>> >>> > On Wed, 19 Jul 2017, Andrew Morton wrote: >>> > >>> > >I do rather dislike these conversions from the point of view of >>> > >performance overhead and general code bloat. But I seem to have lost >>> > >that struggle and I don't think any of these are fastpath(?). >>> > >>> > Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t >>> > implementation), performance is supposed to be ok. >>> >>> Sure, things are OK for people who disable the feature. >> >> So with the WIP fast-refcount series from Kees: >> >> [PATCH v6 0/2] x86: Implement fast refcount overflow protection >> >> I believe the robustness difference between optimized-refcount_t and >> full-refcount_t will be marginal. >> >> I.e. we'll be able to have both higher API safety _and_ performance. >> >>> But for people who want to enable the feature we really should minimize the cost >>> by avoiding blindly converting sites which simply don't need it: simple, safe, >>> old, well-tested code. Why go and slow down such code? Need to apply some >>> common sense here... >> >> It's old, well-tested code _for existing, sane parameters_, until someone finds a >> decade old bug in one of these with an insane parameters no-one stumbled upon so >> far, and builds an exploit on top of it. >> >> Only by touching all these places do we have a chance to improve things measurably >> in terms of reducing the probability of bugs. > > The more I hear people pushing the upsides of refcount_t without > considering the downsides the more I dislike it. > > - refcount_t is really the wrong thing because it uses saturation > semantics. So by definition it includes a bug. This is a feature, not a bug. :) If the kernel has a refcount overflow flaw (which, in the pantheon of exploitable kernel bugs, is _common_[1], as I've referenced earlier), then we're downgrading an exploitable use-after-free to a harmless memory allocation leak. Even if you don't include malicious attackers in the consideration, this changes a memory corruption of unknown results into a memory leak. That's actually an _improvement_ to availability and integrity. > - refcount_t will only really prevent something if there is an extra > increment. That is not the kind of bug people are likely to make. Like I've said, this is common. This is usually a mistake in error handling which forgets (or misplaces) a "put". > - refcount_t won't help if you have an extra decrement. The bad > use-after-free will still happen. Yes, and not having a protected refcount_t will also allow a use-after-free. There is no change here, so it's not a "downside" of refcount_t. In fact, having gained the implicit annotation of refcount_t being a refcounter (rather than a simple atomic_t) means that auditing users is easier and more focused. This could reduce the chance people make mistakes in the first place, especially since the API is more constrained than atomic_t. > - refcount_t won't help if there is a memory stomp. As with an extra > decrement the bad use-after-free will still happen. A stomp of the refcount_t value itself? Sure, and this remains as vulnerable as atomic_t. This isn't a downside to refcount_t. And again, since there _is_ checking of the value in places, it's possible an actionable warning will be produced (though, yes, after the use-after-free has been exposed), which is a benefit over simple atomic_t. I mention this in the commit log ("better to maybe produce the warning than be universally silent"). > So all I see is a huge amount of code churn to implement a buggy (by > definition) refcounting API, that risks adding new bugs and only truly > helps with bugs that are unlikely in the first place. Given that the conversions alone have been uncovering refcount bugs and that the implementation isn't "buggy" (it provides a specific set of protections), I strongly disagree with your assessment. > I really don't think this is an obvious slam dunk. It entirely blocks a commonly exploitable flaw in the kernel. This isn't a probabilistic mitigation, either. While I'm not sure I'd ever describe a security protection as a slam dunk, I think this is up there. :) -Kees [1] When I say "common", I'm speaking from the perspective of security flaw frequency. The kernel sees about 1-2 high severity security flaws a year (with an average lifetime of 5 years), and the refcount-overflow use-after-free class of flaw is normally reliable for attackers (and I'd classify as high severity). With 2016 seeing two known separate refcount-overflow use-after-free flaws, this could be better described as an epidemic, but I'll try to be less inflammatory and just say "common". -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] ipc: convert sem_undo_list.refcnt from atomic_t to refcount_t 2017-07-07 8:59 [PATCH 0/3] v2 ipc subsystem refcount coversions Elena Reshetova 2017-07-07 8:59 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova @ 2017-07-07 8:59 ` Elena Reshetova 2017-07-07 8:59 ` [PATCH 3/3] ipc: convert kern_ipc_perm.refcount " Elena Reshetova 2 siblings, 0 replies; 26+ messages in thread From: Elena Reshetova @ 2017-07-07 8:59 UTC (permalink / raw) To: linux-kernel Cc: peterz, gregkh, akpm, ebiederm, mingo, adobriyan, serge, arozansk, dave, keescook, Elena Reshetova, Hans Liljestrand, David Windsor refcount_t type and corresponding API should be used instead of atomic_t when the variable is used as a reference counter. This allows to avoid accidental refcounter overflows that might lead to use-after-free situations. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: David Windsor <dwindsor@gmail.com> --- ipc/sem.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ipc/sem.c b/ipc/sem.c index 9e70cd7..d3ab140 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -122,7 +122,7 @@ struct sem_undo { * that may be shared among all a CLONE_SYSVSEM task group. */ struct sem_undo_list { - atomic_t refcnt; + refcount_t refcnt; spinlock_t lock; struct list_head list_proc; }; @@ -1642,7 +1642,7 @@ static inline int get_undo_list(struct sem_undo_list **undo_listp) if (undo_list == NULL) return -ENOMEM; spin_lock_init(&undo_list->lock); - atomic_set(&undo_list->refcnt, 1); + refcount_set(&undo_list->refcnt, 1); INIT_LIST_HEAD(&undo_list->list_proc); current->sysvsem.undo_list = undo_list; @@ -2041,7 +2041,7 @@ int copy_semundo(unsigned long clone_flags, struct task_struct *tsk) error = get_undo_list(&undo_list); if (error) return error; - atomic_inc(&undo_list->refcnt); + refcount_inc(&undo_list->refcnt); tsk->sysvsem.undo_list = undo_list; } else tsk->sysvsem.undo_list = NULL; @@ -2070,7 +2070,7 @@ void exit_sem(struct task_struct *tsk) return; tsk->sysvsem.undo_list = NULL; - if (!atomic_dec_and_test(&ulp->refcnt)) + if (!refcount_dec_and_test(&ulp->refcnt)) return; for (;;) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/3] ipc: convert kern_ipc_perm.refcount from atomic_t to refcount_t 2017-07-07 8:59 [PATCH 0/3] v2 ipc subsystem refcount coversions Elena Reshetova 2017-07-07 8:59 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova 2017-07-07 8:59 ` [PATCH 2/3] ipc: convert sem_undo_list.refcnt " Elena Reshetova @ 2017-07-07 8:59 ` Elena Reshetova 2 siblings, 0 replies; 26+ messages in thread From: Elena Reshetova @ 2017-07-07 8:59 UTC (permalink / raw) To: linux-kernel Cc: peterz, gregkh, akpm, ebiederm, mingo, adobriyan, serge, arozansk, dave, keescook, Elena Reshetova, Hans Liljestrand, David Windsor refcount_t type and corresponding API should be used instead of atomic_t when the variable is used as a reference counter. This allows to avoid accidental refcounter overflows that might lead to use-after-free situations. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: David Windsor <dwindsor@gmail.com> --- include/linux/ipc.h | 3 ++- ipc/util.c | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/include/linux/ipc.h b/include/linux/ipc.h index fadd579..ae68980 100644 --- a/include/linux/ipc.h +++ b/include/linux/ipc.h @@ -4,6 +4,7 @@ #include <linux/spinlock.h> #include <linux/uidgid.h> #include <uapi/linux/ipc.h> +#include <linux/refcount.h> #define IPCMNI 32768 /* <= MAX_INT limit for ipc arrays (including sysctl changes) */ @@ -22,7 +23,7 @@ struct kern_ipc_perm { void *security; struct rcu_head rcu; - atomic_t refcount; + refcount_t refcount; } ____cacheline_aligned_in_smp __randomize_layout; #endif /* _LINUX_IPC_H */ diff --git a/ipc/util.c b/ipc/util.c index 1a2cb02..069bb22 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -232,7 +232,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int size) idr_preload(GFP_KERNEL); - atomic_set(&new->refcount, 1); + refcount_set(&new->refcount, 1); spin_lock_init(&new->lock); new->deleted = false; rcu_read_lock(); @@ -397,13 +397,13 @@ void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp) int ipc_rcu_getref(struct kern_ipc_perm *ptr) { - return atomic_inc_not_zero(&ptr->refcount); + return refcount_inc_not_zero(&ptr->refcount); } void ipc_rcu_putref(struct kern_ipc_perm *ptr, void (*func)(struct rcu_head *head)) { - if (!atomic_dec_and_test(&ptr->refcount)) + if (!refcount_dec_and_test(&ptr->refcount)) return; call_rcu(&ptr->rcu, func); -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 0/3] ipc subsystem refcounter conversions @ 2017-02-20 11:29 Elena Reshetova 2017-02-20 11:29 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova 0 siblings, 1 reply; 26+ messages in thread From: Elena Reshetova @ 2017-02-20 11:29 UTC (permalink / raw) To: linux-kernel Cc: peterz, gregkh, akpm, ebiederm, mingo, adobriyan, serge, arozansk, dave, Elena Reshetova Now when new refcount_t type and API are finally merged (see include/linux/refcount.h), the following patches convert various refcounters in the ipc susystem from atomic_t to refcount_t. By doing this we prevent intentional or accidental underflows or overflows that can led to use-after-free vulnerabilities. The below patches are fully independent and can be cherry-picked separately. Since we convert all kernel subsystems in the same fashion, resulting in about 300 patches, we have to group them for sending at least in some fashion to be manageable. Please excuse the long cc list. Elena Reshetova (3): ipc: convert ipc_namespace.count from atomic_t to refcount_t ipc: convert sem_undo_list.refcnt from atomic_t to refcount_t ipc: convert ipc_rcu.refcount from atomic_t to refcount_t include/linux/ipc_namespace.h | 5 +++-- ipc/msgutil.c | 2 +- ipc/namespace.c | 4 ++-- ipc/sem.c | 8 ++++---- ipc/util.c | 6 +++--- ipc/util.h | 3 ++- 6 files changed, 15 insertions(+), 13 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-02-20 11:29 [PATCH 0/3] ipc subsystem refcounter conversions Elena Reshetova @ 2017-02-20 11:29 ` Elena Reshetova 2017-05-27 19:41 ` Kees Cook 0 siblings, 1 reply; 26+ messages in thread From: Elena Reshetova @ 2017-02-20 11:29 UTC (permalink / raw) To: linux-kernel Cc: peterz, gregkh, akpm, ebiederm, mingo, adobriyan, serge, arozansk, dave, Elena Reshetova, Hans Liljestrand, Kees Cook, David Windsor refcount_t type and corresponding API should be used instead of atomic_t when the variable is used as a reference counter. This allows to avoid accidental refcounter overflows that might lead to use-after-free situations. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: David Windsor <dwindsor@gmail.com> --- include/linux/ipc_namespace.h | 5 +++-- ipc/msgutil.c | 2 +- ipc/namespace.c | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h index 848e579..7230638 100644 --- a/include/linux/ipc_namespace.h +++ b/include/linux/ipc_namespace.h @@ -7,6 +7,7 @@ #include <linux/notifier.h> #include <linux/nsproxy.h> #include <linux/ns_common.h> +#include <linux/refcount.h> struct user_namespace; @@ -19,7 +20,7 @@ struct ipc_ids { }; struct ipc_namespace { - atomic_t count; + refcount_t count; struct ipc_ids ids[3]; int sem_ctls[4]; @@ -118,7 +119,7 @@ extern struct ipc_namespace *copy_ipcs(unsigned long flags, static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns) { if (ns) - atomic_inc(&ns->count); + refcount_inc(&ns->count); return ns; } diff --git a/ipc/msgutil.c b/ipc/msgutil.c index bf74eaa..8459802 100644 --- a/ipc/msgutil.c +++ b/ipc/msgutil.c @@ -29,7 +29,7 @@ DEFINE_SPINLOCK(mq_lock); * and not CONFIG_IPC_NS. */ struct ipc_namespace init_ipc_ns = { - .count = ATOMIC_INIT(1), + .count = REFCOUNT_INIT(1), .user_ns = &init_user_ns, .ns.inum = PROC_IPC_INIT_INO, #ifdef CONFIG_IPC_NS diff --git a/ipc/namespace.c b/ipc/namespace.c index 0abdea4..ed10bbc 100644 --- a/ipc/namespace.c +++ b/ipc/namespace.c @@ -48,7 +48,7 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns, goto fail_free; ns->ns.ops = &ipcns_operations; - atomic_set(&ns->count, 1); + refcount_set(&ns->count, 1); ns->user_ns = get_user_ns(user_ns); ns->ucounts = ucounts; @@ -142,7 +142,7 @@ static void free_ipc_ns(struct ipc_namespace *ns) */ void put_ipc_ns(struct ipc_namespace *ns) { - if (atomic_dec_and_lock(&ns->count, &mq_lock)) { + if (refcount_dec_and_lock(&ns->count, &mq_lock)) { mq_clear_sbinfo(ns); spin_unlock(&mq_lock); mq_put_mnt(ns); -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-02-20 11:29 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova @ 2017-05-27 19:41 ` Kees Cook 2017-05-28 12:10 ` Manfred Spraul 0 siblings, 1 reply; 26+ messages in thread From: Kees Cook @ 2017-05-27 19:41 UTC (permalink / raw) To: Andrew Morton, Manfred Spraul Cc: LKML, Elena Reshetova, Peter Zijlstra, Greg KH, Eric W. Biederman, Ingo Molnar, Alexey Dobriyan, Serge E. Hallyn, arozansk, Davidlohr Bueso, Hans Liljestrand, David Windsor On Mon, Feb 20, 2017 at 3:29 AM, Elena Reshetova <elena.reshetova@intel.com> wrote: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > Signed-off-by: David Windsor <dwindsor@gmail.com> Manfred, is this okay by you? I think this should go via -mm... -Kees > --- > include/linux/ipc_namespace.h | 5 +++-- > ipc/msgutil.c | 2 +- > ipc/namespace.c | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h > index 848e579..7230638 100644 > --- a/include/linux/ipc_namespace.h > +++ b/include/linux/ipc_namespace.h > @@ -7,6 +7,7 @@ > #include <linux/notifier.h> > #include <linux/nsproxy.h> > #include <linux/ns_common.h> > +#include <linux/refcount.h> > > struct user_namespace; > > @@ -19,7 +20,7 @@ struct ipc_ids { > }; > > struct ipc_namespace { > - atomic_t count; > + refcount_t count; > struct ipc_ids ids[3]; > > int sem_ctls[4]; > @@ -118,7 +119,7 @@ extern struct ipc_namespace *copy_ipcs(unsigned long flags, > static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns) > { > if (ns) > - atomic_inc(&ns->count); > + refcount_inc(&ns->count); > return ns; > } > > diff --git a/ipc/msgutil.c b/ipc/msgutil.c > index bf74eaa..8459802 100644 > --- a/ipc/msgutil.c > +++ b/ipc/msgutil.c > @@ -29,7 +29,7 @@ DEFINE_SPINLOCK(mq_lock); > * and not CONFIG_IPC_NS. > */ > struct ipc_namespace init_ipc_ns = { > - .count = ATOMIC_INIT(1), > + .count = REFCOUNT_INIT(1), > .user_ns = &init_user_ns, > .ns.inum = PROC_IPC_INIT_INO, > #ifdef CONFIG_IPC_NS > diff --git a/ipc/namespace.c b/ipc/namespace.c > index 0abdea4..ed10bbc 100644 > --- a/ipc/namespace.c > +++ b/ipc/namespace.c > @@ -48,7 +48,7 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns, > goto fail_free; > ns->ns.ops = &ipcns_operations; > > - atomic_set(&ns->count, 1); > + refcount_set(&ns->count, 1); > ns->user_ns = get_user_ns(user_ns); > ns->ucounts = ucounts; > > @@ -142,7 +142,7 @@ static void free_ipc_ns(struct ipc_namespace *ns) > */ > void put_ipc_ns(struct ipc_namespace *ns) > { > - if (atomic_dec_and_lock(&ns->count, &mq_lock)) { > + if (refcount_dec_and_lock(&ns->count, &mq_lock)) { > mq_clear_sbinfo(ns); > spin_unlock(&mq_lock); > mq_put_mnt(ns); > -- > 2.7.4 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t 2017-05-27 19:41 ` Kees Cook @ 2017-05-28 12:10 ` Manfred Spraul 0 siblings, 0 replies; 26+ messages in thread From: Manfred Spraul @ 2017-05-28 12:10 UTC (permalink / raw) To: Kees Cook, Andrew Morton Cc: LKML, Elena Reshetova, Peter Zijlstra, Greg KH, Eric W. Biederman, Ingo Molnar, Alexey Dobriyan, Serge E. Hallyn, arozansk, Davidlohr Bueso, Hans Liljestrand, David Windsor On 05/27/2017 09:41 PM, Kees Cook wrote: > On Mon, Feb 20, 2017 at 3:29 AM, Elena Reshetova > <elena.reshetova@intel.com> wrote: >> refcount_t type and corresponding API should be >> used instead of atomic_t when the variable is used as >> a reference counter. This allows to avoid accidental >> refcounter overflows that might lead to use-after-free >> situations. >> >> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> >> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Signed-off-by: David Windsor <dwindsor@gmail.com> > Manfred, is this okay by you? I think this should go via -mm... The patch is ok, the refcounters are not in the critical path. I'll try to test the patches and then I would send an 'Acked-by', but this will take some days. -- Manfred ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-07-20 15:12 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-07 8:59 [PATCH 0/3] v2 ipc subsystem refcount coversions Elena Reshetova 2017-07-07 8:59 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova 2017-07-09 21:59 ` Eric W. Biederman 2017-07-10 6:48 ` Reshetova, Elena 2017-07-10 8:37 ` Eric W. Biederman 2017-07-10 9:34 ` Alexey Dobriyan 2017-07-10 11:19 ` Eric W. Biederman 2017-07-10 9:56 ` Reshetova, Elena 2017-07-10 11:26 ` Eric W. Biederman 2017-07-10 12:11 ` Reshetova, Elena 2017-07-10 20:32 ` Eric W. Biederman 2017-07-12 9:21 ` Reshetova, Elena 2017-07-19 22:35 ` Andrew Morton 2017-07-19 22:54 ` Davidlohr Bueso 2017-07-19 22:58 ` Andrew Morton 2017-07-19 23:11 ` Davidlohr Bueso 2017-07-19 23:20 ` Kees Cook 2017-07-20 0:32 ` Kees Cook 2017-07-20 9:34 ` Ingo Molnar 2017-07-20 12:34 ` Eric W. Biederman 2017-07-20 15:12 ` Kees Cook 2017-07-07 8:59 ` [PATCH 2/3] ipc: convert sem_undo_list.refcnt " Elena Reshetova 2017-07-07 8:59 ` [PATCH 3/3] ipc: convert kern_ipc_perm.refcount " Elena Reshetova -- strict thread matches above, loose matches on Subject: below -- 2017-02-20 11:29 [PATCH 0/3] ipc subsystem refcounter conversions Elena Reshetova 2017-02-20 11:29 ` [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Elena Reshetova 2017-05-27 19:41 ` Kees Cook 2017-05-28 12:10 ` Manfred Spraul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).