| Message ID | 1487590730-18352-4-git-send-email-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Elena Reshetova 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> > --- > security/tomoyo/common.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) Thanks for proposal, but I don't think this patch is correct. tomoyo_query_observers remembers number of "struct file" referring /sys/kernel/security/tomoyo/query interface. Therefore, it is OK to increment when the value is 0 (and overflow does not matter, for if there were so many open files, the system will be already OOM). Please exclude this patch from your series. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 27, 2017 at 5:45 PM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > Elena Reshetova 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> >> --- >> security/tomoyo/common.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) > > Thanks for proposal, but I don't think this patch is correct. > > tomoyo_query_observers remembers number of "struct file" referring > /sys/kernel/security/tomoyo/query interface. Therefore, it is OK to > increment when the value is 0 (and overflow does not matter, for > if there were so many open files, the system will be already OOM). The issue is about killing bug classes. Many use-after-free refcounting bugs are a result of unbalanced get/put, rather than wrapping due to successful gets. (See the recent keyctl CVE: failure path missed a put, so get could wrap, etc.) > Please exclude this patch from your series. Could the refcounting be adjusted with +1 so that the 0->1 becomes 1->2 instead? -Kees
Kees Cook wrote: > On Mon, Feb 27, 2017 at 5:45 PM, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > Elena Reshetova 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> > >> --- > >> security/tomoyo/common.c | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > > > Thanks for proposal, but I don't think this patch is correct. > > > > tomoyo_query_observers remembers number of "struct file" referring > > /sys/kernel/security/tomoyo/query interface. Therefore, it is OK to > > increment when the value is 0 (and overflow does not matter, for > > if there were so many open files, the system will be already OOM). > > The issue is about killing bug classes. Many use-after-free > refcounting bugs are a result of unbalanced get/put, rather than > wrapping due to successful gets. (See the recent keyctl CVE: failure > path missed a put, so get could wrap, etc.) > > > Please exclude this patch from your series. > > Could the refcounting be adjusted with +1 so that the 0->1 becomes 1->2 instead? That changes nothing. tomoyo_query_observers is nothing but a shortcut for rejecting access requests (without waiting for 10 seconds) when nobody is listening on /sys/kernel/security/tomoyo/query interface. There is no use-after-free with tomoyo_query_observers because it does not involve memory allocation/free. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Kees Cook wrote: > > On Mon, Feb 27, 2017 at 5:45 PM, Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > Elena Reshetova 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> > > >> --- > > >> security/tomoyo/common.c | 11 ++++++----- > > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > Thanks for proposal, but I don't think this patch is correct. > > > > > > tomoyo_query_observers remembers number of "struct file" referring > > > /sys/kernel/security/tomoyo/query interface. Therefore, it is OK to > > > increment when the value is 0 (and overflow does not matter, for > > > if there were so many open files, the system will be already OOM). > > > > The issue is about killing bug classes. Many use-after-free > > refcounting bugs are a result of unbalanced get/put, rather than > > wrapping due to successful gets. (See the recent keyctl CVE: failure > > path missed a put, so get could wrap, etc.) > > > > > Please exclude this patch from your series. > > > > Could the refcounting be adjusted with +1 so that the 0->1 becomes 1->2 > instead? > > That changes nothing. tomoyo_query_observers is nothing but a shortcut for > rejecting access requests (without waiting for 10 seconds) when nobody is > listening on /sys/kernel/security/tomoyo/query interface. There is no use- > after-free > with tomoyo_query_observers because it does not involve memory > allocation/free. Sounds reasonable, thank you very much for checking the patch! I will drop it from series. In general we tried to submit for reviews even the cases where we were not 100% sure if conversion should be made or not in order to get maintainer feedback and make sure we don't misjudge anything by ourselves. Best Regards, Elena. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c index e0fb750..9a7605a 100644 --- a/security/tomoyo/common.c +++ b/security/tomoyo/common.c @@ -7,6 +7,7 @@ #include <linux/uaccess.h> #include <linux/slab.h> #include <linux/security.h> +#include <linux/refcount.h> #include "common.h" /* String table for operation mode. */ @@ -1907,7 +1908,7 @@ static DEFINE_SPINLOCK(tomoyo_query_list_lock); * Number of "struct file" referring /sys/kernel/security/tomoyo/query * interface. */ -static atomic_t tomoyo_query_observers = ATOMIC_INIT(0); +static refcount_t tomoyo_query_observers = REFCOUNT_INIT(0); /** * tomoyo_truncate - Truncate a line. @@ -2015,7 +2016,7 @@ int tomoyo_supervisor(struct tomoyo_request_info *r, const char *fmt, ...) switch (r->mode) { case TOMOYO_CONFIG_ENFORCING: error = -EPERM; - if (atomic_read(&tomoyo_query_observers)) + if (refcount_read(&tomoyo_query_observers)) break; goto out; case TOMOYO_CONFIG_LEARNING: @@ -2059,7 +2060,7 @@ int tomoyo_supervisor(struct tomoyo_request_info *r, const char *fmt, ...) wake_up_all(&tomoyo_query_wait); if (wait_event_interruptible_timeout (tomoyo_answer_wait, entry.answer || - !atomic_read(&tomoyo_query_observers), HZ)) + !refcount_read(&tomoyo_query_observers), HZ)) break; else entry.timer++; @@ -2437,7 +2438,7 @@ int tomoyo_open_control(const u8 type, struct file *file) * there is some process monitoring /sys/kernel/security/tomoyo/query. */ if (type == TOMOYO_QUERY) - atomic_inc(&tomoyo_query_observers); + refcount_inc(&tomoyo_query_observers); file->private_data = head; tomoyo_notify_gc(head, true); return 0; @@ -2687,7 +2688,7 @@ void tomoyo_close_control(struct tomoyo_io_buffer *head) * observer counter. */ if (head->type == TOMOYO_QUERY && - atomic_dec_and_test(&tomoyo_query_observers)) + refcount_dec_and_test(&tomoyo_query_observers)) wake_up_all(&tomoyo_answer_wait); tomoyo_notify_gc(head, false); }