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+Yrq4mt6c8wQKU7WcTnN7k28T3hM1V6_DWF-NpmuMH7Gw@mail.gmail.com>
Date:	Mon, 2 May 2016 13:21:13 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Chen Gang <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 1:11 PM, Chen Gang <chengang@...ndsoft.com.cn> wrote:
> 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.

Signed counter looks good to me.
We can both issue a WARNING and prevent the actual overflow/underflow.
But I don't think that there is any sane way to handle the bug (other
than just fixing the unmatched disable/enable). If we ignore an
excessive disable, then we can end up with ignores enabled
permanently. If we ignore an excessive enable, then we can end up with
ignores enabled when they should not be enabled. The main point here
is to bark loudly, so that the unmatched annotations are noticed and
fixed.