llvm dev - Apr 2018 - [cfe-dev] RFC: Implementing -fno-delete-null-pointer-checks in clang

> On Apr 20, 2018, at 2:06 AM, Csaba Raduly via cfe-dev <cfe-dev at
lists.llvm.org> wrote:
> 
> On 4/20/18, James Y Knight wrote:
>> 
>> 
>> Yep. "-fnull-pointer-is-valid" has been suggested before.
>> 
> 
> -fplacate-linux-kernel-developers ?
Please, lets keep this discussion on topic and productive.  The semantics
described upstream of (the inverse of) "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at address
zero.” is entirely reasonable.

-Chris

On 21 Apr 2018, at 22:19, Chris Lattner via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
>> 
>> On Apr 20, 2018, at 2:06 AM, Csaba Raduly via cfe-dev <cfe-dev at
lists.llvm.org> wrote:
>> 
>> On 4/20/18, James Y Knight wrote:
>>> 
>>> 
>>> Yep. "-fnull-pointer-is-valid" has been suggested before.
>>> 
>> 
>> -fplacate-linux-kernel-developers ?
> 
> Please, lets keep this discussion on topic and productive.  The semantics
described upstream of (the inverse of) "Assume that programs cannot safely
dereference null pointers, and that no code or data element resides at address
zero.” is entirely reasonable.
I disagree, because it depends on what you mean by dereference.  If it is safe
to dereference NULL, then that means that it is also safe to hoist the null
dereference above the check.  For example:

	if (x == NULL)
		return;
	y = *x;

Is safe to transform into:

	y = *x;
	if (x == NULL)
		return;

The load is guaranteed not to trap by the fact that NULL a dereferencable
address.

As I understand it, the issue in GCC was that they treated the address
calculation as a dereference.  The word ‘dereference’ does not appear in the C
standard, which talks only about use, and therefore treats *x and &x->y
both as uses.  This is intentional, because on segmented architectures it is
possible that NULL + X will give you an out-of-bounds and unrepresentable
pointer, possibly even causing a trap.

A Linux kernel compiled with clang was not vulnerable, because LLVM was not
eliding this check, which makes this flag somewhat pointless.  LLVM assumed that
a reachable load or a store of an pointer guaranteed that it was not NULL, but
that a GEP did not.

I believe that the correct fix is to explicitly permit GEPs with a NULL base in
the LLVM IR spec.  As I understand it, this is already implicitly permitted
because code that predates __builtin_offsetof uses a cast of 0 to a struct type
and takes the address of a field to get the offset.

Any code that is loading or storing from null prior to a null check probably
doesn’t mind if the null check is then elided, because it’s going to trap anyway
(supporting C code that allows loads and stores from an address with a bit
pattern of 0 when interpreted as an integer is incredibly hard, as our friends
at IBM can attest).

David

On Sun, Apr 22, 2018, 5:11 AM David Chisnall via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> I disagree, because it depends on what you mean by dereference.  If it is
> safe to dereference NULL, then that means that it is also safe to hoist the
> null dereference above the check.  For example:
>
Nope. The intent is that NULL is *potentially* dereferenceable, just as any
other address would be. Not that it's known to *always* be dereferenceable.

        if (x == NULL)
>                 return;
>         y = *x;
>
> Is safe to transform into:
>
>         y = *x;
>         if (x == NULL)
>                 return;
>
> The load is guaranteed not to trap by the fact that NULL a dereferencable
> address.

That transform is still not safe with the new flag, because you do not know
that address 0 is dereferenceable in that code. You also don't know that
it's definitely _not_ dereferenceable, however -- which is the new behavior.

> Any code that is loading or storing from null prior to a null check
> probably doesn’t mind if the null check is then elided, because it’s going
> to trap anyway (supporting C code that allows loads and stores from an
> address with a bit pattern of 0 when interpreted as an integer is
> incredibly hard, as our friends at IBM can attest).
>
No -- the kernel *does* mind exactly this, because in some circumstances,
dereferencing null *does not trap* in kernel context. So you end up with
both no trap and no check, and a security vulnerability. That's less true
now, with the various other protections e.g. min_mmap_address sysctl, SMAP,
and others, but still, the desire is to keep this from happening.

Here is the commit which started using the flag in the kernel:

commit a3ca86aea507904148870946d599e07a340b39bf

Author: Eugene Teo <eteo at redhat.com>

Date:   Wed Jul 15 14:59:10 2009 +0800


    Add '-fno-delete-null-pointer-checks' to gcc CFLAGS



    Turning on this flag could prevent the compiler from optimising away

    some "useless" checks for null pointers.  Such bugs can sometimes
become

    exploitable at compile time because of the -O2 optimisation.



    See http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Optimize-Options.html



    An example that clearly shows this 'problem' is commit 6bf67672.



     static void __devexit agnx_pci_remove(struct pci_dev *pdev)

     {

         struct ieee80211_hw *dev = pci_get_drvdata(pdev);

    -    struct agnx_priv *priv = dev->priv;

    +    struct agnx_priv *priv;

         AGNX_TRACE;



         if (!dev)

             return;

    +    priv = dev->priv;



    By reverting this patch, and compile it with and without

    -fno-delete-null-pointer-checks flag, we can see that the check for dev

    is compiled away.



        call    printk  #

    -   testq   %r12, %r12  # dev

    -   je  .L94    #,

        movq    %r12, %rdi  # dev,



    Clearly the 'fix' is to stop using dev before it is tested, but
building

    with -fno-delete-null-pointer-checks flag at least makes it harder to

    abuse.


                                                    Signed-off-by: Eugene
Teo <eugeneteo at kernel.sg>

    Acked-by: Eric Paris <eparis at redhat.com>

    Acked-by: Wang Cong <amwang at redhat.com>

    Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180423/70b52b68/attachment.html>