| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <201401102215.IEJ51533.FFVQHOOLOFJSMt@I-love.SAKURA.ne.jp>
Date: Fri, 10 Jan 2014 22:15:06 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: pavel@....cz
Cc: keescook@...omium.org, joe@...ches.com, akpm@...ux-foundation.org,
geert@...ux-m68k.org, jkosina@...e.cz, viro@...iv.linux.org.uk,
davem@...emloft.net, linux-kernel@...r.kernel.org,
selinux@...ho.nsa.gov, linux-security-module@...r.kernel.org
Subject: Re: [PATCH] lib/vsprintf: add %pT[C012] format specifier
Tetsuo Handa wrote:
> Pavel Machek wrote:
> > > + * Please use this wrapper function which will be updated in the future to read
> > > + * @tsk->comm in a consistent way.
> > > + */
> > > +static inline int commcmp(const struct task_struct *tsk, const char *comm)
> > > +{
> > > + return strcmp(tsk->comm, comm);
> > > +}
> >
> > Is this useful to something? Printing command name is
> > useful. Comparing it...?
> >
>
> (a) Using tsk->comm for reducing duplicated printk() messages.
>
> if (strcmp(p->comm, last_comm)) {
> printk("hello\n");
> strcpy(last_comm, p->comm);
> }
>
> (b) Using tsk->pid for reducing duplicated printk() messages.
>
> if (p->pid != last_pid) {
> printk("hello\n");
> last_pid = p->pid;
> }
>
> (c) Using printk_ratelimit() for reducing printk() flooding.
>
> printk_ratelimit("hello\n");
>
> (d) Using plain printk().
>
> printk("hello\n");
>
> (e) Other purposes. (e.g. drivers/target/iscsi/iscsi_target_tq.c )
>
> if (!strncmp(current->comm, ISCSI_RX_THREAD_NAME,
> strlen(ISCSI_RX_THREAD_NAME)))
> thread_called = ISCSI_RX_THREAD;
> else if (!strncmp(current->comm, ISCSI_TX_THREAD_NAME,
> strlen(ISCSI_TX_THREAD_NAME)))
> thread_called = ISCSI_TX_THREAD;
>
> commcmp() and commcpy() are for wrappring (a).
> Though (a) should consider changing to (b) or (c).
>
> (e) should be rewritten not to depend on current->comm .
>
I realized that we don't need commcmp() because we can rewrite like below.
(a)
char tmp_comm[TASK_COMM_LEN];
commcpy(tmp_comm, p->comm);
if (strcmp(tmp_comm, last_comm)) {
printk("hello\n");
strcpy(last_comm, tmp_comm);
}
(e)
char tmp_comm[TASK_COMM_LEN];
commcpy(tmp_comm, p->comm);
if (!strncmp(tmp_comm, ISCSI_RX_THREAD_NAME,
strlen(ISCSI_RX_THREAD_NAME)))
thread_called = ISCSI_RX_THREAD;
else if (!strncmp(tmp_comm, ISCSI_TX_THREAD_NAME,
strlen(ISCSI_TX_THREAD_NAME)))
thread_called = ISCSI_TX_THREAD;
Below is an updated patch (and one of users of this patch). I assume we can
agree on these patches, and I expect these patches go to linux-next.git via
linux-security.git (as I can save one merge window for fixing this issue in
the LSM auditing code.)
----------------------------------------
>>From 0c183ad7aceb0db8ca4b9bd3a048182bf23e32be Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Date: Fri, 10 Jan 2014 21:54:38 +0900
Subject: [PATCH] exec: Add wrapper function for reading task_struct->comm.
Since task_struct->comm can be modified by other threads while the current
thread is reading it, it is recommended to use get_task_comm() for reading it.
However, since get_task_comm() holds task_struct->alloc_lock spinlock,
some users cannot use get_task_comm(). Also, a lot of users are directly
reading from task_struct->comm even if they can use get_task_comm().
Such users might obtain inconsistent result.
This patch introduces a wrapper function for reading task_struct->comm .
Currently this function does not provide consistency. I'm planning to change to
use RCU in the future. By using RCU, the comm name read from task_struct->comm
will be guaranteed to be consistent. But before modifying set_task_comm() to
use RCU, we need to kill direct ->comm users who do not use get_task_comm().
Users directly reading from task_struct->comm for printing purpose can use
%pT format specifier rather than this wrapper function.
Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
---
include/linux/sched.h | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index e27baee..de12b27 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1601,6 +1601,24 @@ static inline cputime_t task_gtime(struct task_struct *t)
extern void task_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st);
extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st);
+/**
+ * commcpy - Copy task_struct->comm to buffer.
+ *
+ * @buf: Buffer to copy @tsk->comm which must be at least TASK_COMM_LEN bytes.
+ * @tsk: Pointer to "struct task_struct".
+ *
+ * Returns return value of memcpy(@buf, @tsk->comm, TASK_COMM_LEN).
+ *
+ * Please use get_task_comm(@buf, @tsk) if it is safe to call task_lock(@tsk),
+ * for get_task_comm() reads @tsk->comm in a consistent way. Otherwise, please
+ * use this wrapper function which will be updated in the future to read
+ * @tsk->comm in a consistent way.
+ */
+static inline void *commcpy(void *buf, const struct task_struct *tsk)
+{
+ return memcpy(buf, tsk->comm, TASK_COMM_LEN);
+}
+
/*
* Per process flags
*/
--
1.7.1
----------------------------------------
>>From 3b5137f09bfdac7b0133ddaa194d181d7c897e1f Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Date: Fri, 10 Jan 2014 22:02:14 +0900
Subject: [PATCH] LSM: Pass comm name via commcpy()
When we pass task->comm to audit_log_untrustedstring(), we need to pass a
snapshot of it using commcpy() because task->comm can be changed to contain
control character by other threads after audit_log_untrustedstring() confirmed
that task->comm does not contain control character.
Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
---
security/lsm_audit.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 8d8d97d..76f37f7 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -212,6 +212,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
struct common_audit_data *a)
{
struct task_struct *tsk = current;
+ char name[TASK_COMM_LEN];
/*
* To keep stack sizes in check force programers to notice if they
@@ -221,7 +222,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
audit_log_format(ab, " pid=%d comm=", tsk->pid);
- audit_log_untrustedstring(ab, tsk->comm);
+ audit_log_untrustedstring(ab, commcpy(name, tsk));
switch (a->type) {
case LSM_AUDIT_DATA_NONE:
@@ -280,7 +281,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
tsk = a->u.tsk;
if (tsk && tsk->pid) {
audit_log_format(ab, " pid=%d comm=", tsk->pid);
- audit_log_untrustedstring(ab, tsk->comm);
+ audit_log_untrustedstring(ab, commcpy(name, tsk));
}
break;
case LSM_AUDIT_DATA_NET:
--
1.7.1
----------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists