linux-kernel - Re: [PATCH] Change task

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 25 Feb 2014 21:54:01 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	laijs@...fujitsu.com
Cc:	akpm@...ux-foundation.org, joe@...ches.com, keescook@...omium.org,
	geert@...ux-m68k.org, jkosina@...e.cz, viro@...iv.linux.org.uk,
	davem@...emloft.net, linux-kernel@...r.kernel.org, mingo@...e.hu,
	peterz@...radead.org, rostedt@...dmis.org, tglx@...utronix.de
Subject: Re: [PATCH] Change task_struct->comm to use RCU.

Lai Jiangshan wrote:
> CC scheduler people.
> 
> I can't figure out what we get with this patch.
> 
OK. Welcome to this thread. I'll explain you what is going on.

Current problem:

  printk("%s\n", task->comm) is racy because "%s" format specifier assumes that
  the corresponding argument does not change between strnlen() and the for loop
  at string() in lib/vsnprintf.c . If task->comm was "Hello Linux" until
  strnlen() and becomes "Penguin" before the for loop, "%s" will emit
  "Penguin\0nux" (note the unexpected '\0' byte and the garbage bytes).

  Likewise, audit_log_untrustedstring(ab, current->comm) is racy.
  If task->comm was "Hello Linux" until audit_string_contains_control() in
  audit_log_n_untrustedstring() returns false, and becomes "Penguin" before
  memcpy() in audit_log_n_string() is called, memcpy() will emit "Penguin\0nux"
  into the audit log, which results in loss of information (e.g. SELinux
  context) due to the unexpected '\0' byte.

Proposed solution:

  To fix abovementioned problem, I proposed commcpy() and "%pT" format
  specifier which does

    char tmp[16];
    memcpy(tmp, task->comm, 16);
    tmp[15] = '\0';
    sprintf(buf, "%s", tmp);

  instead of

    sprintf(buf, "%s", task->comm);

  .

Remaining problem:

  Although the proposed solution will prevent the caller from emitting the
  unexpected '\0' byte and the garbage bytes, memcpy(tmp, task->comm, 16) in
  the proposed solution is not atomic. That is, "%pT" does not emit the '\0'
  byte like "Penguin\0nux" but "%pT" still might emit "Penguininux".

  To fix this problem, I proposed protecting memcpy(tmp, task->comm, 16) part
  using RCU. This patch is a design for how the update side of task->comm will
  look like if we use RCU approach.

  Of course, this approach depends on that nobody prefers the speed of reading
  task->comm over the atomicity of reading task->comm . If somebody strongly
  objects on the cost of calling rcu_read_lock()/rcu_read_unlock() for the
  atomicity, I'm fine without this patch.
--
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/