On Thu, Jun 8, 2023 at 9:47 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> I am a little worried about how (any version so far of) this API could go
> wrong, e.g. if someone uses this and does "return p" instead of "return
> no_free_ptr(p)",
Absolutely. I think the whole "don't always cleanup" is quite
dangerous, and maybe not worth it, but it _is_ a fairly common
pattern.
> I was hoping we could do
> something like this to the end of automatic_kfree_wrapper():
>
> *(void **)pp = NULL;
That would be a lovely thing, but as you found out, it fundamentally
cannot work.
The whole point of "cleanup" is that it is done when the variable -
not trh *value* - goes out of scope.
So when you have that
return var; /* oops, forgot to disable cleanup */
situation, by definition "var" hasn't gone out of scope until _after_
you read the value and return that value, so pretty much by definition
it cannot make a difference to assign something to 'pp' in the cleanup
code.
> The point being, if we can proactively make this hard to shoot ourselves in
> the foot, that would be nice. :)
So the good thing is that things like KASAN would immediately notice,
and since this all happens (presumably) for the success case, it's not
about some unlikely error case.
I also think that -fanalyzer might just catch it (as part of
-Wanalyzer-use-after-free) at compile-time, but I didn't check. So
if/when people start using -fanalyze on the kernel, those things would
be caught early.
So while this "forget to avoid cleanup" is a worry, I think it ends up
likely being one that is fairly easy to avoid with other checks in
place.
Famous last words.
Linus
