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

[James Y Knight via cfe-dev]

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/cfe-dev/attachments/20180423/70b52b68/attachment.html>