[3/3] security, tomoyo: convert tomoyo_query_observers from atomic_t to refcount_t
diff mbox

Message ID 1487590730-18352-4-git-send-email-elena.reshetova@intel.com
State New
Headers show

Commit Message

Reshetova, Elena Feb. 20, 2017, 11:38 a.m. UTC
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(-)

Comments

Tetsuo Handa Feb. 28, 2017, 1:45 a.m. UTC | #1
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
Kees Cook Feb. 28, 2017, 2:28 a.m. UTC | #2
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
Tetsuo Handa Feb. 28, 2017, 2:43 a.m. UTC | #3
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
Reshetova, Elena Feb. 28, 2017, 7:22 a.m. UTC | #4
> 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

Patch
diff mbox

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);
 }