| Message ID | 157851809847.1732.8255673984543824278.stgit@ubuntu3-kvm2 |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Fixes for sockmap/tls from more complex BPF progs | expand |
On Wed, Jan 8, 2020 at 1:15 PM John Fastabend <john.fastabend@gmail.com> wrote: > > In the push, pull, and pop helpers operating on skmsg objects to make > data writable or insert/remove data we use this bounds check to ensure > specified data is valid, > > /* Bounds checks: start and pop must be inside message */ > if (start >= offset + l || last >= msg->sg.size) > return -EINVAL; > > The problem here is offset has already included the length of the > current element the 'l' above. So start could be past the end of > the scatterlist element in the case where start also points into an > offset on the last skmsg element. > > To fix do the accounting slightly different by adding the length of > the previous entry to offset at the start of the iteration. And > ensure its initialized to zero so that the first iteration does > nothing. > > Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") > Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data") > Fixes: 7246d8ed4dcce ("bpf: helper to pop data from messages") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> This is pretty tricky... But it looks right. Acked-by: Song Liu <songliubraving@fb.com>
diff --git a/net/core/filter.c b/net/core/filter.c index 28b3c258188c..34d8eb0823f4 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -2231,10 +2231,10 @@ BPF_CALL_4(bpf_msg_pull_data, struct sk_msg *, msg, u32, start, /* First find the starting scatterlist element */ i = msg->sg.start; do { + offset += len; len = sk_msg_elem(msg, i)->length; if (start < offset + len) break; - offset += len; sk_msg_iter_var_next(i); } while (i != msg->sg.end); @@ -2346,7 +2346,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start, u32, len, u64, flags) { struct scatterlist sge, nsge, nnsge, rsge = {0}, *psge; - u32 new, i = 0, l, space, copy = 0, offset = 0; + u32 new, i = 0, l = 0, space, copy = 0, offset = 0; u8 *raw, *to, *from; struct page *page; @@ -2356,11 +2356,11 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start, /* First find the starting scatterlist element */ i = msg->sg.start; do { + offset += l; l = sk_msg_elem(msg, i)->length; if (start < offset + l) break; - offset += l; sk_msg_iter_var_next(i); } while (i != msg->sg.end); @@ -2506,7 +2506,7 @@ static void sk_msg_shift_right(struct sk_msg *msg, int i) BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start, u32, len, u64, flags) { - u32 i = 0, l, space, offset = 0; + u32 i = 0, l = 0, space, offset = 0; u64 last = start + len; int pop; @@ -2516,11 +2516,11 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start, /* First find the starting scatterlist element */ i = msg->sg.start; do { + offset += l; l = sk_msg_elem(msg, i)->length; if (start < offset + l) break; - offset += l; sk_msg_iter_var_next(i); } while (i != msg->sg.end);
In the push, pull, and pop helpers operating on skmsg objects to make data writable or insert/remove data we use this bounds check to ensure specified data is valid, /* Bounds checks: start and pop must be inside message */ if (start >= offset + l || last >= msg->sg.size) return -EINVAL; The problem here is offset has already included the length of the current element the 'l' above. So start could be past the end of the scatterlist element in the case where start also points into an offset on the last skmsg element. To fix do the accounting slightly different by adding the length of the previous entry to offset at the start of the iteration. And ensure its initialized to zero so that the first iteration does nothing. Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") Fixes: 6fff607e2f14b ("bpf: sk_msg program helper bpf_msg_push_data") Fixes: 7246d8ed4dcce ("bpf: helper to pop data from messages") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- net/core/filter.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)