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]

Date:	Mon, 02 May 2016 19:11:39 +0800
From:	Chen Gang <chengang@...ndsoft.com.cn>
To:	Dmitry Vyukov <dvyukov@...gle.com>
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 5/2/16 16:26, Dmitry Vyukov wrote:
> 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.
>>
> 
> Hi Chen,
> 
> I don't think this is correct.

OK, thanks.

> 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.
> 

For me, I guess, what you said above is reasonable.

But it is really strange to any newbies (e.g. me), so it will be better
to get another member's confirmation, too. If no any additional reply by
any other members within 3 days, I shall treat what you said is OK.

> 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.
> 

For me, when WARNING on something, it will dummy the related feature
which should be used (may let user's hope fail), but should not get the
negative result (hurt user's original work). So in our case:

 - When caller calls kasan_report_enabled(), kasan_depth-- to 0, 

 - When a caller calls kasan_report_enabled() again, the caller will get
   a warning, and notice about this calling is failed, but it is still
   in enable state, should not change to disable state automatically.

 - If we report an warning, but still kasan_depth--, it will let things
   much complex.

And for me, another improvements can be done:

 - signed int kasan_depth may be a little better. When kasan_depth > 0,
   it is in disable state, else in enable state. It will be much harder
   to generate overflow than unsigned int kasan_depth.

 - Let kasan_[en|dis]able_current() return Boolean value to notify the
   caller whether the calling succeeds or fails.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.