On Tue, Oct 26, 2021 at 10:51 AM Hao Luo [off-list ref] wrote:

On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Mon, Oct 25, 2021 at 4:13 PM Hao Luo [off-list ref] wrote:

quoted

Some helper functions may modify its arguments, for example,
bpf_d_path, bpf_get_stack etc. Previously, their argument types
were marked as ARG_PTR_TO_MEM, which is compatible with read-only
mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate
to modify a read-only memory by passing it into one of such helper
functions.

This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to
annotate the arguments that may be modified by the helpers. For
arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type,
while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not
acceptable.

In short, when a helper may modify its input parameter, use
ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM.

This is inconsistent with the other uses where we have something
that's writable by default and mark it as RDONLY if it's read-only.
Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and
add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the
memory? It will also be safer default: if helper defines
ARG_PTR_TO_MEM but never writes to it, worst thing that can happen
would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it
without fixing helper definition. The other way is more dangerous. If
ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction
and actually writes into the memory, then we are in much bigger
trouble.

My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I
find it's not intuitive for developers when adding helpers. The
majority of PTR_TO_MEM arguments are read-only. When adding a new
helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM)
to match the default case (that is, read-only argument). Requiring
explicitly adding RDONLY to most cases seems a little unintuitive to
me.

But other than that, I agree that narrowing ARG_PTR_TO_MEM down to
writable memory fosters more strict checks and safer behavior. But
when people add helpers, they are clearly aware which argument will be
modified and which will not. IMHO I do trust that the developers and
the reviewers can choose the right type at the review time. :)

I don't trust myself, and neither should you :) See 5b029a32cfe4
("bpf: Fix ringbuf helper function compatibility") as an example of
the things that shouldn't have happened, but slipped through my own
testing and code review anyway. And there were multiple cases where we
accidentally enabled stuff that we shouldn't or didn't check something
that should have been prevented.

All that is to say that if we can have safer behavior by default (not
as enforced by humans), then it's better. In this sense,
ARG_PTR_TO_MEM meaning writable access is safer, because even if we
accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst
thing is that users won't be able to use helper in some situation and
hopefully will report this and we'll fix it. The alternative is that a
helper declares the argument as read-only memory (accidentally,
because it's shorter enum and sort of default), but actually does the
write to that memory. Now we have a much bigger issue.

quoted

quoted

So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM
is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is
only used in bpf_iter prog as the type of key, which hasn't been
used in the affected helper functions. PTR_TO_RDONLY_MEM currently
has no consumers.

Signed-off-by: Hao Luo <redacted>
---
 Changes since v1:
  - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate
    read-only and read-write mem arg types.

[...]