Add support for pointer to mem register spilling, to allow the verifier
to track pointer to valid memory addresses. Such pointers are returned
for example by a successful call of the bpf_ringbuf_reserve helper.
This patch was suggested as a solution by Yonghong Song.
The patch was partially contibuted by CyberArk Software, Inc.
Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
support for it")
Signed-off-by: Gilad Reti <[email protected]>
---
kernel/bpf/verifier.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 17270b8404f1..36af69fac591 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
case PTR_TO_RDWR_BUF:
case PTR_TO_RDWR_BUF_OR_NULL:
case PTR_TO_PERCPU_BTF_ID:
+ case PTR_TO_MEM:
+ case PTR_TO_MEM_OR_NULL:
return true;
default:
return false;
--
2.27.0
On Tue, Jan 12, 2021 at 10:14 AM Gilad Reti <[email protected]> wrote:
>
> Add support for pointer to mem register spilling, to allow the verifier
> to track pointer to valid memory addresses. Such pointers are returned
nit: pointers
> for example by a successful call of the bpf_ringbuf_reserve helper.
>
> This patch was suggested as a solution by Yonghong Song.
You can use the "Suggested-by:" tag for this.
>
> The patch was partially contibuted by CyberArk Software, Inc.
nit: typo *contributed
Also, I was wondering if "partially" here means someone collaborated with you
on the patch? And, in that case:
"Co-developed-by:" would be a better tag here.
Acked-by: KP Singh <[email protected]>
>
> Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> support for it")
> Signed-off-by: Gilad Reti <[email protected]>
> ---
> kernel/bpf/verifier.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 17270b8404f1..36af69fac591 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
> case PTR_TO_RDWR_BUF:
> case PTR_TO_RDWR_BUF_OR_NULL:
> case PTR_TO_PERCPU_BTF_ID:
> + case PTR_TO_MEM:
> + case PTR_TO_MEM_OR_NULL:
> return true;
> default:
> return false;
> --
> 2.27.0
>
On Tue, Jan 12, 2021 at 3:57 PM KP Singh <[email protected]> wrote:
>
> On Tue, Jan 12, 2021 at 10:14 AM Gilad Reti <[email protected]> wrote:
> >
> > Add support for pointer to mem register spilling, to allow the verifier
> > to track pointer to valid memory addresses. Such pointers are returned
>
> nit: pointers
Thanks
>
> > for example by a successful call of the bpf_ringbuf_reserve helper.
> >
> > This patch was suggested as a solution by Yonghong Song.
>
> You can use the "Suggested-by:" tag for this.
Thanks
>
> >
> > The patch was partially contibuted by CyberArk Software, Inc.
>
> nit: typo *contributed
Thanks. Should I submit a v2 of the patch to correct all of those?
>
> Also, I was wondering if "partially" here means someone collaborated with you
> on the patch? And, in that case:
>
> "Co-developed-by:" would be a better tag here.
No, I did it alone. I mentioned CyberArk since I work there and did some of the
coding during my daily work, so they deserve credit.
>
> Acked-by: KP Singh <[email protected]>
>
>
> >
> > Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> > support for it")
> > Signed-off-by: Gilad Reti <[email protected]>
> > ---
> > kernel/bpf/verifier.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 17270b8404f1..36af69fac591 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
> > case PTR_TO_RDWR_BUF:
> > case PTR_TO_RDWR_BUF_OR_NULL:
> > case PTR_TO_PERCPU_BTF_ID:
> > + case PTR_TO_MEM:
> > + case PTR_TO_MEM_OR_NULL:
> > return true;
> > default:
> > return false;
> > --
> > 2.27.0
> >
On Tue, Jan 12, 2021 at 3:24 PM Gilad Reti <[email protected]> wrote:
>
> On Tue, Jan 12, 2021 at 3:57 PM KP Singh <[email protected]> wrote:
> >
> > On Tue, Jan 12, 2021 at 10:14 AM Gilad Reti <[email protected]> wrote:
> > >
> > > Add support for pointer to mem register spilling, to allow the verifier
> > > to track pointer to valid memory addresses. Such pointers are returned
> >
> > nit: pointers
>
> Thanks
>
> >
> > > for example by a successful call of the bpf_ringbuf_reserve helper.
> > >
> > > This patch was suggested as a solution by Yonghong Song.
> >
> > You can use the "Suggested-by:" tag for this.
>
> Thanks
>
> >
> > >
> > > The patch was partially contibuted by CyberArk Software, Inc.
> >
> > nit: typo *contributed
>
> Thanks. Should I submit a v2 of the patch to correct all of those?
I think it would be nice to do another revision
which also addresses the comments on the other patch.
>
> >
> > Also, I was wondering if "partially" here means someone collaborated with you
> > on the patch? And, in that case:
> >
> > "Co-developed-by:" would be a better tag here.
>
> No, I did it alone. I mentioned CyberArk since I work there and did some of the
> coding during my daily work, so they deserve credit.
>
> >
> > Acked-by: KP Singh <[email protected]>
> >
> >
> > >
> > > Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> > > support for it")
> > > Signed-off-by: Gilad Reti <[email protected]>
> > > ---
> > > kernel/bpf/verifier.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 17270b8404f1..36af69fac591 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
> > > case PTR_TO_RDWR_BUF:
> > > case PTR_TO_RDWR_BUF_OR_NULL:
> > > case PTR_TO_PERCPU_BTF_ID:
> > > + case PTR_TO_MEM:
> > > + case PTR_TO_MEM_OR_NULL:
> > > return true;
> > > default:
> > > return false;
> > > --
> > > 2.27.0
> > >
On Tue, Jan 12, 2021 at 1:14 AM Gilad Reti <[email protected]> wrote:
>
> Add support for pointer to mem register spilling, to allow the verifier
> to track pointer to valid memory addresses. Such pointers are returned
> for example by a successful call of the bpf_ringbuf_reserve helper.
>
> This patch was suggested as a solution by Yonghong Song.
>
> The patch was partially contibuted by CyberArk Software, Inc.
>
> Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
> support for it")
Surprised no one mentioned this yet. Fixes tag should always be on a
single unwrapped line, however long it is, please fix.
> Signed-off-by: Gilad Reti <[email protected]>
> ---
> kernel/bpf/verifier.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 17270b8404f1..36af69fac591 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
> case PTR_TO_RDWR_BUF:
> case PTR_TO_RDWR_BUF_OR_NULL:
> case PTR_TO_PERCPU_BTF_ID:
> + case PTR_TO_MEM:
> + case PTR_TO_MEM_OR_NULL:
> return true;
> default:
> return false;
> --
> 2.27.0
>
Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.
The patch was partially contibuted by CyberArk Software, Inc.
Signed-off-by: Gilad Reti <[email protected]>
---
tools/testing/selftests/bpf/test_verifier.c | 12 +++++++-
.../selftests/bpf/verifier/spill_fill.c | 30 +++++++++++++++++++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 777a81404fdb..f8569f04064b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -50,7 +50,7 @@
#define MAX_INSNS BPF_MAXINSNS
#define MAX_TEST_INSNS 1000000
#define MAX_FIXUPS 8
-#define MAX_NR_MAPS 20
+#define MAX_NR_MAPS 21
#define MAX_TEST_RUNS 8
#define POINTER_VALUE 0xcafe4all
#define TEST_DATA_LEN 64
@@ -87,6 +87,7 @@ struct bpf_test {
int fixup_sk_storage_map[MAX_FIXUPS];
int fixup_map_event_output[MAX_FIXUPS];
int fixup_map_reuseport_array[MAX_FIXUPS];
+ int fixup_map_ringbuf[MAX_FIXUPS];
const char *errstr;
const char *errstr_unpriv;
uint32_t insn_processed;
@@ -640,6 +641,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
int *fixup_sk_storage_map = test->fixup_sk_storage_map;
int *fixup_map_event_output = test->fixup_map_event_output;
int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
+ int *fixup_map_ringbuf = test->fixup_map_ringbuf;
if (test->fill_helper) {
test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -817,6 +819,14 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
fixup_map_reuseport_array++;
} while (*fixup_map_reuseport_array);
}
+ if (*fixup_map_ringbuf) {
+ map_fds[20] = create_map(BPF_MAP_TYPE_RINGBUF, 0,
+ 0, 4096);
+ do {
+ prog[*fixup_map_ringbuf].imm = map_fds[20];
+ fixup_map_ringbuf++;
+ } while (*fixup_map_ringbuf);
+ }
}
struct libcap {
diff --git a/tools/testing/selftests/bpf/verifier/spill_fill.c b/tools/testing/selftests/bpf/verifier/spill_fill.c
index 45d43bf82f26..1833b6c730dd 100644
--- a/tools/testing/selftests/bpf/verifier/spill_fill.c
+++ b/tools/testing/selftests/bpf/verifier/spill_fill.c
@@ -28,6 +28,36 @@
.result = ACCEPT,
.result_unpriv = ACCEPT,
},
+{
+ "check valid spill/fill, ptr to mem",
+ .insns = {
+ /* reserve 8 byte ringbuf memory */
+ BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_MOV64_IMM(BPF_REG_2, 8),
+ BPF_MOV64_IMM(BPF_REG_3, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve),
+ /* store a pointer to the reserved memory in R6 */
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ /* check whether the reservation was successful */
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
+ /* spill R6(mem) into the stack */
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, -8),
+ /* fill it back in R7 */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, -8),
+ /* should be able to access *(R7) = 0 */
+ BPF_ST_MEM(BPF_DW, BPF_REG_7, 0, 0),
+ /* submit the reserved rungbuf memory */
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+ BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_submit),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_ringbuf = { 1 },
+ .result = ACCEPT,
+ .result_unpriv = ACCEPT,
+},
{
"check corrupted spill/fill",
.insns = {
--
2.27.0
On 1/12/21 8:46 PM, Andrii Nakryiko wrote:
> On Tue, Jan 12, 2021 at 1:14 AM Gilad Reti <[email protected]> wrote:
>>
>> Add support for pointer to mem register spilling, to allow the verifier
>> to track pointer to valid memory addresses. Such pointers are returned
>> for example by a successful call of the bpf_ringbuf_reserve helper.
>>
>> This patch was suggested as a solution by Yonghong Song.
>>
>> The patch was partially contibuted by CyberArk Software, Inc.
>>
>> Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier
>> support for it")
>
> Surprised no one mentioned this yet. Fixes tag should always be on a
> single unwrapped line, however long it is, please fix.
Especially for first-time contributors there is usually some luxury that we
would have fixed this up on the fly while applying. ;) But given a v2 is going
to be sent anyway it's good to point it out for future reference, agree.
Thanks,
Daniel