On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> > >
> > > 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.
>
> > >
> > > 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 <haoluo@xxxxxxxxxx>
> > > ---
> > > Changes since v1:
> > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate
> > > read-only and read-write mem arg types.
> > >
[...]
