linux-kernel - Re: [PATCH] debugfs: Check return value of debugfs_real

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:   Wed, 28 Mar 2018 07:47:53 -0700
From:   Manoj Gupta <manojgupta@...omium.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Matthias Kaehlcke <mka@...omium.org>, linux-kernel@...r.kernel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

Please note that there is nothing wrong in the generated code, just
that it confuses objtool.
Clang has simply omitted the statement where NULL is returned since
the pointer was always dereferenced post inlining.

Note that GCC will also remove the NULL pointers if it knows that the
pointer is dereferenced.
Here is an example.

void null_check(int *P) {
  int deref = *P;
  if (P == 0) // GCC won't check the condition.
    return;
  *P = 4;
}

Compiling with gcc -O2 gives:
        movl    $4, (%rdi)
        ret

Thanks,
Manoj

On Tue, Mar 27, 2018 at 11:16 PM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Tue, Mar 27, 2018 at 04:55:53PM -0700, Matthias Kaehlcke wrote:
>> debugfs_real_fops() returns a NULL pointer when it is invoked without a
>> prior call to debugfs_file_get(). In code paths including this call it
>> is not strictly necessary to check the return value of
>> debugfs_real_fops(). However clang inlines debugfs_real_fops(), detects
>> the invalid dereferencing of the NULL pointer and drops the code path.
>
> Wait, what?  Why would it do that, because it thinks dereferencing NULL
> is undefined behaviour and it can just do whatever it wants to?
>
> That feels crazy, as for these calls we "know" it will never be NULL
> because the previous call to debugfs_file_get() will always ensure it
> will be correct.
>
> So this is a case of the compiler trying to be smarter than it really
> is, and getting things totally wrong :(
>
> Has anyone reported this to the clang developers?
>
> Papering over compiler foolishness is not something I like to do in
> kernel code if at all possible...
>
> thanks,
>
> greg k-h