| Message ID | 20211109003052.3499225-1-haoluo@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: Prevent writing read-only memory | expand |
On Mon, Nov 8, 2021 at 4:31 PM Hao Luo <haoluo@google.com> wrote: > > There are currently two ways to modify a kernel memory in bpf programs: > 1. declare a ksym of scalar type and directly modify its memory. > 2. Pass a RDONLY_BUF into a helper function which will override > its arguments. For example, bpf_d_path, bpf_snprintf. > > This patchset fixes these two problem. For the first, we introduce a > new reg type PTR_TO_RDONLY_MEM for the scalar typed ksym, which forbids > writing. For the second, we introduce a new arg type ARG_CONST_PTR_TO_MEM > to differentiate the arg types that only read the memory from those > that may write the memory. The previous ARG_PTR_TO_MEM is now only > compatible with writable memories. If a helper doesn't write into its > argument, it can use ARG_CONST_PTR_TO_MEM, which is also compatible > with read-only memories. > > In v2, Andrii suggested using the name "ARG_PTR_TO_RDONLY_MEM", but I > find it is sort of misleading. Because the new arg_type is compatible > with both write and read-only memory. So I chose ARG_CONST_PTR_TO_MEM > instead. I find ARG_CONST_PTR_TO_MEM misleading. It's the difference between `char * const` (const pointer to mutable memory) vs `const char *` (pointer to an immutable memory). We need the latter semantics, and that *is* PTR_TO_RDONLY_MEM in BPF verifier terms. Drawing further analogies from C, you can pass `char *` (pointer to mutable memory) to any function that expects `const char *`, because it's safe to do so, but not the other way. So I don't think it's confusing at all that it is PTR_TO_RDONLY_MEM and that you can pass PTR_TO_MEM register to a helper that expects ARG_PTR_TO_RDONLY_MEM. > > Hao Luo (3): > bpf: Prevent write to ksym memory > bpf: Introduce ARG_CONST_PTR_TO_MEM > bpf/selftests: Test PTR_TO_RDONLY_MEM > > include/linux/bpf.h | 20 +++++- > include/uapi/linux/bpf.h | 4 +- > kernel/bpf/btf.c | 2 +- > kernel/bpf/cgroup.c | 2 +- > kernel/bpf/helpers.c | 12 ++-- > kernel/bpf/ringbuf.c | 2 +- > kernel/bpf/syscall.c | 2 +- > kernel/bpf/verifier.c | 60 +++++++++++++---- > kernel/trace/bpf_trace.c | 26 ++++---- > net/core/filter.c | 64 +++++++++---------- > tools/include/uapi/linux/bpf.h | 4 +- > .../selftests/bpf/prog_tests/ksyms_btf.c | 14 ++++ > .../bpf/progs/test_ksyms_btf_write_check.c | 29 +++++++++ > 13 files changed, 168 insertions(+), 73 deletions(-) > create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c > > -- > 2.34.0.rc0.344.g81b53c2807-goog >
On Tue, Nov 9, 2021 at 8:43 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Nov 8, 2021 at 4:31 PM Hao Luo <haoluo@google.com> wrote: > > > > There are currently two ways to modify a kernel memory in bpf programs: > > 1. declare a ksym of scalar type and directly modify its memory. > > 2. Pass a RDONLY_BUF into a helper function which will override > > its arguments. For example, bpf_d_path, bpf_snprintf. > > > > This patchset fixes these two problem. For the first, we introduce a > > new reg type PTR_TO_RDONLY_MEM for the scalar typed ksym, which forbids > > writing. For the second, we introduce a new arg type ARG_CONST_PTR_TO_MEM > > to differentiate the arg types that only read the memory from those > > that may write the memory. The previous ARG_PTR_TO_MEM is now only > > compatible with writable memories. If a helper doesn't write into its > > argument, it can use ARG_CONST_PTR_TO_MEM, which is also compatible > > with read-only memories. > > > > In v2, Andrii suggested using the name "ARG_PTR_TO_RDONLY_MEM", but I > > find it is sort of misleading. Because the new arg_type is compatible > > with both write and read-only memory. So I chose ARG_CONST_PTR_TO_MEM > > instead. > > I find ARG_CONST_PTR_TO_MEM misleading. It's the difference between > `char * const` (const pointer to mutable memory) vs `const char *` > (pointer to an immutable memory). We need the latter semantics, and > that *is* PTR_TO_RDONLY_MEM in BPF verifier terms. > Ah, I am aware of the semantic difference between 'char * const' and 'const char *', but your explanation in the bracket helps me see your point better. It does seem PTR_TO_RDONLY_MEM matches the semantics now. Let me fix and send an update. > Drawing further analogies from C, you can pass `char *` (pointer to > mutable memory) to any function that expects `const char *`, because > it's safe to do so, but not the other way. > > So I don't think it's confusing at all that it is PTR_TO_RDONLY_MEM > and that you can pass PTR_TO_MEM register to a helper that expects > ARG_PTR_TO_RDONLY_MEM. > > > > > Hao Luo (3): > > bpf: Prevent write to ksym memory > > bpf: Introduce ARG_CONST_PTR_TO_MEM > > bpf/selftests: Test PTR_TO_RDONLY_MEM > > > > include/linux/bpf.h | 20 +++++- > > include/uapi/linux/bpf.h | 4 +- > > kernel/bpf/btf.c | 2 +- > > kernel/bpf/cgroup.c | 2 +- > > kernel/bpf/helpers.c | 12 ++-- > > kernel/bpf/ringbuf.c | 2 +- > > kernel/bpf/syscall.c | 2 +- > > kernel/bpf/verifier.c | 60 +++++++++++++---- > > kernel/trace/bpf_trace.c | 26 ++++---- > > net/core/filter.c | 64 +++++++++---------- > > tools/include/uapi/linux/bpf.h | 4 +- > > .../selftests/bpf/prog_tests/ksyms_btf.c | 14 ++++ > > .../bpf/progs/test_ksyms_btf_write_check.c | 29 +++++++++ > > 13 files changed, 168 insertions(+), 73 deletions(-) > > create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf_write_check.c > > > > -- > > 2.34.0.rc0.344.g81b53c2807-goog > >
On Wed, Nov 10, 2021 at 11:55 AM Hao Luo <haoluo@google.com> wrote: > > On Tue, Nov 9, 2021 at 8:43 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Nov 8, 2021 at 4:31 PM Hao Luo <haoluo@google.com> wrote: > > > > > > There are currently two ways to modify a kernel memory in bpf programs: > > > 1. declare a ksym of scalar type and directly modify its memory. > > > 2. Pass a RDONLY_BUF into a helper function which will override > > > its arguments. For example, bpf_d_path, bpf_snprintf. > > > > > > This patchset fixes these two problem. For the first, we introduce a > > > new reg type PTR_TO_RDONLY_MEM for the scalar typed ksym, which forbids > > > writing. For the second, we introduce a new arg type ARG_CONST_PTR_TO_MEM > > > to differentiate the arg types that only read the memory from those > > > that may write the memory. The previous ARG_PTR_TO_MEM is now only > > > compatible with writable memories. If a helper doesn't write into its > > > argument, it can use ARG_CONST_PTR_TO_MEM, which is also compatible > > > with read-only memories. > > > > > > In v2, Andrii suggested using the name "ARG_PTR_TO_RDONLY_MEM", but I > > > find it is sort of misleading. Because the new arg_type is compatible > > > with both write and read-only memory. So I chose ARG_CONST_PTR_TO_MEM > > > instead. > > > > I find ARG_CONST_PTR_TO_MEM misleading. It's the difference between > > `char * const` (const pointer to mutable memory) vs `const char *` > > (pointer to an immutable memory). We need the latter semantics, and > > that *is* PTR_TO_RDONLY_MEM in BPF verifier terms. > > > > Ah, I am aware of the semantic difference between 'char * const' and > 'const char *', but your explanation in the bracket helps me see your > point better. It does seem PTR_TO_RDONLY_MEM matches the semantics > now. Let me fix and send an update. I thought earlier we agreed that flag approach is prefered. Looks like OR_NULL discussion is progressing nicely and IS_RDONLY or IS_RDWR flags will just fit right in. What's the reason to go with this approach ? It seems it will make the refactoring more tedious later.
On Wed, Nov 10, 2021 at 2:05 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Nov 10, 2021 at 11:55 AM Hao Luo <haoluo@google.com> wrote: > > > > On Tue, Nov 9, 2021 at 8:43 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Mon, Nov 8, 2021 at 4:31 PM Hao Luo <haoluo@google.com> wrote: > > > > > > > > There are currently two ways to modify a kernel memory in bpf programs: > > > > 1. declare a ksym of scalar type and directly modify its memory. > > > > 2. Pass a RDONLY_BUF into a helper function which will override > > > > its arguments. For example, bpf_d_path, bpf_snprintf. > > > > > > > > This patchset fixes these two problem. For the first, we introduce a > > > > new reg type PTR_TO_RDONLY_MEM for the scalar typed ksym, which forbids > > > > writing. For the second, we introduce a new arg type ARG_CONST_PTR_TO_MEM > > > > to differentiate the arg types that only read the memory from those > > > > that may write the memory. The previous ARG_PTR_TO_MEM is now only > > > > compatible with writable memories. If a helper doesn't write into its > > > > argument, it can use ARG_CONST_PTR_TO_MEM, which is also compatible > > > > with read-only memories. > > > > > > > > In v2, Andrii suggested using the name "ARG_PTR_TO_RDONLY_MEM", but I > > > > find it is sort of misleading. Because the new arg_type is compatible > > > > with both write and read-only memory. So I chose ARG_CONST_PTR_TO_MEM > > > > instead. > > > > > > I find ARG_CONST_PTR_TO_MEM misleading. It's the difference between > > > `char * const` (const pointer to mutable memory) vs `const char *` > > > (pointer to an immutable memory). We need the latter semantics, and > > > that *is* PTR_TO_RDONLY_MEM in BPF verifier terms. > > > > > > > Ah, I am aware of the semantic difference between 'char * const' and > > 'const char *', but your explanation in the bracket helps me see your > > point better. It does seem PTR_TO_RDONLY_MEM matches the semantics > > now. Let me fix and send an update. > > I thought earlier we agreed that flag approach is prefered. > Looks like OR_NULL discussion is progressing nicely and > IS_RDONLY or IS_RDWR flags will just fit right in. > What's the reason to go with this approach ? > It seems it will make the refactoring more tedious later. I felt this patch series has been holding for too long and is now mostly ready. In contrast, I wasn't sure how many iterations are needed for the flag patch and kind of worried it may further delay this patch, so I developed these two patches independently. I can certainly merge them into one if that's preferred.