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

Hi,

This is regarding support for -fno-delete-null-pointer-checks in clang (PR
9251).
Linux kernel uses this flag to keep null pointer checks from getting
optimized away.  Since clang does not currently support this flag, it often
invites comments from kernel devs that clang is not yet *ready* to compile
Linux kernel.
I have also heard that developers working on firmware, bare-metal tools and
other low level tools often want to use this flag as well.

Therefore, I would like to implement support for this flag (maybe with a
different name), and would like to know the opinions on how it should be
implemented.

Some options/opinions:

1 (From Duncan Sands comment at PR 9251)
> This could be implemented by putting pointers in a non-default address
space when this flag is passed.
Any ideas how will this work for languages/targets that actively make use
of non-default address spaces e.g. OpenCL/GPU targets.  Also, I believe
that allocas still need to kept in default address space so they need to be
bitcast to the *no-default* address space before use.

2. Use this flag like any other optimization flag. Find and fix all
optimizations in clang/llvm related to null pointer accesses.

Thanks,
Manoj

Background:
https://lkml.org/lkml/2018/4/4/601

 From Linus Torvalds:

Note that we explicitly use "-fno-delete-null-pointer-checks" because
we do *not* want NULL pointer check removal even in the case of a bug.

See commit a3ca86aea507 ("Add '-fno-delete-null-pointer-checks' to
gcc
CFLAGS") for the reason: we had buggy code that accessed a pointer
before the NULL pointer check, but the bug was "benign" as long as the
compiler didn't actually remove the check.

And note how the buggy code *looked* safe. It was doing the right
check, it was just that the check was hidden and disabled by another
bug.

Removing the NULL pointer check turned a benign bug into a trivially
exploitable one by just mapping user space data at NULL (which avoided
the kernel oops, and then made the kernel use the user value!).

Now, admittedly we have a ton of other security features in place to
avoid these kinds of issues - SMAP helps on the hardware side, and not
allowing users to mmap() NULL in the first place helps with most
distributions, but the point remains: the kernel generally really
doesn't want optimizations that are perhaps allowed by the standard,
but that result in code generation that doesn't match the source code.

The NULL pointer removal is one such thing: don't remove checks in the
kernel based on "standard says". It's ok to do optimizations that
are
based on "hardware does the exact same thing", but not on "the
standard says this is undefined so we can remove it".

On 18 April 2018 at 18:13, Manoj Gupta via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
> Therefore, I would like to implement support for this flag (maybe with a
> different name),
I'd suggest -mdo-what-i-mean; the whole idea is horribly
underspecified, and basically rips up the LangRef in favour of a
nebulous set of good and bad optimizations (probably as dictated by
the ones that have bitten someone who wrote bad code recently and was
grumpy enough about it to complain at us).

In particular I'm skeptical that the address space solution actually
works. What they actually mean is probably not so much about removing
the icmps, but about the control-flow decisions based on them. And
LLVM might not be aware that any given conditional branch comes from a
tainted comparison.

Cheers.

Tim.

On 4/18/2018 11:21 AM, Tim Northover via cfe-dev wrote:
> On 18 April 2018 at 18:13, Manoj Gupta via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
>> Therefore, I would like to implement support for this flag (maybe with
a
>> different name),
> I'd suggest -mdo-what-i-mean; the whole idea is horribly
> underspecified, and basically rips up the LangRef in favour of a
> nebulous set of good and bad optimizations (probably as dictated by
> the ones that have bitten someone who wrote bad code recently and was
> grumpy enough about it to complain at us).
Despite the name, the flag actually has rather straightforward semantics 
from the compiler's perspective.  From the gcc docs for 
-fdelete-null-pointer-checks: "Assume that programs cannot safely 
dereference null pointers, and that no code or data element resides at 
address zero."  (-fno-delete-null-pointer-checks is the opposite.)

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project

Hi,

Turning this on its head. Adding a warning saying something like
"Warning: NULL point check being optimized out at line X"  might be
useful.
The developer might spot that and think "Ah! maybe I put that check in
the wrong place!"

Kind Regards

James



On 18 April 2018 at 19:21, Tim Northover via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
> On 18 April 2018 at 18:13, Manoj Gupta via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
>> Therefore, I would like to implement support for this flag (maybe with
a
>> different name),
>
> I'd suggest -mdo-what-i-mean; the whole idea is horribly
> underspecified, and basically rips up the LangRef in favour of a
> nebulous set of good and bad optimizations (probably as dictated by
> the ones that have bitten someone who wrote bad code recently and was
> grumpy enough about it to complain at us).
>
> In particular I'm skeptical that the address space solution actually
> works. What they actually mean is probably not so much about removing
> the icmps, but about the control-flow decisions based on them. And
> LLVM might not be aware that any given conditional branch comes from a
> tainted comparison.
>
> Cheers.
>
> Tim.
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

Fair warning, the following is a devil's advocate position, but it's 
also a serious question.

Given the entire point of this flag appears to be bug mitigation, why 
not frame this as a sanitizer?  If we had a hypothetical 
-fsanitize=dereference which tried to catch dereferenced pointers 
derived from null, wouldn't that handle the case at hand?

i.e. Why are we focused on *hiding* the bug instead of *finding and 
fixing* the bug?

Philip

p.s. The above is written with the understanding that the code compiled 
with this flag doesn’t actually place valid objects at null.  I know 
that is not true for some embedded systems, but that seems distinct from 
this proposal.

On 04/18/2018 10:13 AM, Manoj Gupta via llvm-dev wrote:
> Hi,
>
> This is regarding support for -fno-delete-null-pointer-checks in clang (PR
> 9251).
> Linux kernel uses this flag to keep null pointer checks from getting
> optimized away.  Since clang does not currently support this flag, it often
> invites comments from kernel devs that clang is not yet *ready* to compile
> Linux kernel.
> I have also heard that developers working on firmware, bare-metal tools and
> other low level tools often want to use this flag as well.
>
> Therefore, I would like to implement support for this flag (maybe with a
> different name), and would like to know the opinions on how it should be
> implemented.
>
> Some options/opinions:
>
> 1 (From Duncan Sands comment at PR 9251)
>> This could be implemented by putting pointers in a non-default address
> space when this flag is passed.
> Any ideas how will this work for languages/targets that actively make use
> of non-default address spaces e.g. OpenCL/GPU targets.  Also, I believe
> that allocas still need to kept in default address space so they need to be
> bitcast to the *no-default* address space before use.
>
> 2. Use this flag like any other optimization flag. Find and fix all
> optimizations in clang/llvm related to null pointer accesses.
>
> Thanks,
> Manoj
>
> Background:
> https://lkml.org/lkml/2018/4/4/601
>
>   From Linus Torvalds:
>
> Note that we explicitly use "-fno-delete-null-pointer-checks"
because
> we do *not* want NULL pointer check removal even in the case of a bug.
>
> See commit a3ca86aea507 ("Add
'-fno-delete-null-pointer-checks' to gcc
> CFLAGS") for the reason: we had buggy code that accessed a pointer
> before the NULL pointer check, but the bug was "benign" as long
as the
> compiler didn't actually remove the check.
>
> And note how the buggy code *looked* safe. It was doing the right
> check, it was just that the check was hidden and disabled by another
> bug.
>
> Removing the NULL pointer check turned a benign bug into a trivially
> exploitable one by just mapping user space data at NULL (which avoided
> the kernel oops, and then made the kernel use the user value!).
>
> Now, admittedly we have a ton of other security features in place to
> avoid these kinds of issues - SMAP helps on the hardware side, and not
> allowing users to mmap() NULL in the first place helps with most
> distributions, but the point remains: the kernel generally really
> doesn't want optimizations that are perhaps allowed by the standard,
> but that result in code generation that doesn't match the source code.
>
> The NULL pointer removal is one such thing: don't remove checks in the
> kernel based on "standard says". It's ok to do optimizations
that are
> based on "hardware does the exact same thing", but not on
"the
> standard says this is undefined so we can remove it".
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On 5/12/2018 9:23 PM, Philip Reames via llvm-dev wrote:
> Fair warning, the following is a devil's advocate position, but
it's
> also a serious question.
>
> Given the entire point of this flag appears to be bug mitigation, why 
> not frame this as a sanitizer?  If we had a hypothetical 
> -fsanitize=dereference which tried to catch dereferenced pointers 
> derived from null, wouldn't that handle the case at hand?
It's called "-fsanitize=null": it catches stuff like
"x[3]" where x is
null. It's not quite complete; we don't check for arithmetic on a null 
pointer.

Yes, that would handle the situation in question, but putting implicit 
null checks all over the place is pretty expensive; I don't think most 
people would turn that on in production.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project