linux-kernel - Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report

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]

Message-ID: <CACT4Y+Z7Yfsq9wjJuoeegEvPBvJs9iX6wN2VO1scA7HA4TVLmQ@mail.gmail.com>
Date:	Mon, 2 May 2016 10:26:09 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	chengang@...ndsoft.com.cn
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Alexander Potapenko <glider@...gle.com>,
	kasan-dev <kasan-dev@...glegroups.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Chen Gang <gang.chen.5i5j@...il.com>
Subject: Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()

On Mon, May 2, 2016 at 7:36 AM,  <chengang@...ndsoft.com.cn> wrote:
> From: Chen Gang <chengang@...ndsoft.com.cn>
>
> According to kasan_[dis|en]able_current() comments and the kasan_depth'
> s initialization, if kasan_depth is zero, it means disable.
>
> So need use "!!kasan_depth" instead of "!kasan_depth" for checking
> enable.
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@...il.com>
> ---
>  mm/kasan/kasan.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..6464b8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -102,7 +102,7 @@ static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
>
>  static inline bool kasan_report_enabled(void)
>  {
> -       return !current->kasan_depth;
> +       return !!current->kasan_depth;
>  }
>
>  void kasan_report(unsigned long addr, size_t size,

Hi Chen,

I don't think this is correct.
We seem to have some incorrect comments around kasan_depth, and a
weird way of manipulating it (disable should increment, and enable
should decrement). But in the end it is working. This change will
suppress all true reports and enable all false reports.

If you want to improve kasan_depth handling, then please fix the
comments and make disable increment and enable decrement (potentially
with WARNING on overflow/underflow). It's better to produce a WARNING
rather than silently ignore the error. We've ate enough unmatched
annotations in user space (e.g. enable is skipped on an error path).
These unmatched annotations are hard to notice (they suppress
reports). So in user space we bark loudly on overflows/underflows and
also check that a thread does not exit with enabled suppressions.

Thanks.