linux-kernel - Re: [GIT PULL] ext4 changes for the 6.4 merge window

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOd=mgAMuMODXTapt8JRqEFLS1j-hfssZE0YjJNjPhH=H5A@mail.gmail.com>
Date:   Fri, 28 Apr 2023 14:02:56 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     "Theodore Ts'o" <tytso@....edu>,
        Nathan Chancellor <nathan@...nel.org>,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Kees Cook <keescook@...omium.org>
Subject: Re: [GIT PULL] ext4 changes for the 6.4 merge window

On Wed, Apr 26, 2023 at 3:31 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Wed, Apr 26, 2023 at 3:08 PM Nick Desaulniers
> <ndesaulniers@...gle.com> wrote:
> >
> > Is this what you had in mind?
> > ```
> > void * _Nonnull foo (void)
> > ...
> > void bar (void) {
> >     if (foo() == NULL) // maybe should warn that foo() returns _Nonnull?
> >         bar();
> > ...
> > linus.c:8:15: warning: comparison of _Nonnull function call 'foo'
> > equal to a null pointer is always false
>
> Yes.
>
> HOWEVER.
>
> I suspect you will find that it gets complicated for more indirect
> uses, and that may be why people have punted on this.
>
> For example, let's say that you instead have
>
>    void *bar(void) { return foo(); }
>
> and 'bar()' gets inlined.
>
> The obvious reaction to that is "ok, clearly the result is still
> _Nonnull, and should warn if it is tested.
>
> But that obvious reaction is actually completely wrong, because it may
> be that the real code looks something like
>
>    void *bar(void) {
> #if CONFIG_XYZ
>     if (somecondition) return NULL;
> #endif
>     return foo(); }
>
> and the caller really *should* check for NULL - it's just that the
> compiler never saw that case.

I think having a return value be conditionally _Nonnull is "garbage
in; garbage out."  The straightforward case would be to have `bar` be
conditionally _Nonnull on the same preprocessor condition.

void *
#ifndef CONFIG_XYZ
_Nonnull
#endif
bar (void) {
#ifdef CONFIG_XYZ
  if (somecondition) return NULL;
#endif
  return foo(); }

Then code reviewers could go: "yikes; please make bar unconditionally
_Nonnull, or simply don't use _Nonnull here."

>
> So only testing the direct return value of a function should warn.
>
> And even that "direct return value" is not trivial. What happens if
> you have something like this:
>
>    void bar(void) { do_something(foo()); }
>
> and "do_something()" ends up being inlined - and checks for its
> argument for being NULL? Again, that "test against NULL" may well be
> absolutely required in that context - because *other* call-sites will
> pass in pointers that might be NULL.
>
> Now, I don't know how clang works internally, but I suspect based just
> on the size of your patch that your patch would get all of this
> horribly wrong.

Of course, it was a quick hack.

>
> So doing a naked
>
>     void *ptr = foo();
>     if (!ptr) ...
>
> should warn.
>
> But doing the exact same thing, except the test for NULL came in some
> other context that just got inlined, cannot warn.

Thinking more about this, I really think _Nonnull should behave like a
qualifier (const, restrict, volatile). So the above example would be:

void * _Nonnull ptr = foo();
if (!ptr) // warning: tautology

That would require changes to the variables that calls to functions
that return ERR_PTR's were stored in.  That simplifies the semantic
analysis, since the compiler can simply look at the declaration of
`ptr` and not try to see that `ptr`'s value at some point in the
control flow graph was something returned from calling a _Nonnull
returning function.

Because _Nonnull isn't modeled as a qualifier in clang today, this doesn't warn:
```
void foo(void* _Nonnull);
void *bar(void);

void baz (void) {
  void *x = bar();
  foo(x);
}
```
It would be nice to say that "baz calls foo which expects its
parameter to be _Nonnull, but you passed a pointer that could be
null".  I also wonder if casting works...

Rust got this right; pretty sure they have NonNull and NonZero types.


>
> I _suspect_ that the reason clang does what it does is that this is
> just very complicated to do well.
>
> It sounds easy to a human, but ...

The bones are there; we could finish the damned thing if it sounds
like something that would be useful/usable in the kernel?  I guess the
impetus is that ERR_PTR checks should be comparing against < 0 rather
than == NULL, since that's a tautology?

-- 
Thanks,
~Nick Desaulniers