| Message ID | 157851817088.1732.14988301389495595092.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:17 PM John Fastabend <john.fastabend@gmail.com> wrote: > > When passed a zero length skmsg tls_push_record() causes a NULL ptr > deref. To resolve for fixes do a simple length check at start of > routine. Could you please include the stack dump for the NULL deref? Thanks, Song > > To create this case a user can create a BPF program to pop all the > data off the message then return SK_PASS. Its not a very practical > or useful thing to do so we mark it unlikely. > > Fixes: d3b18ad31f93d ("tls: add bpf support to sk_msg handling") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- > net/tls/tls_sw.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 21c7725d17ca..0326e916ab01 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -680,6 +680,9 @@ static int tls_push_record(struct sock *sk, int flags, > msg_pl = &rec->msg_plaintext; > msg_en = &rec->msg_encrypted; > > + if (unlikely(!msg_pl->sg.size)) > + return 0; > + > split_point = msg_pl->apply_bytes; > split = split_point && split_point < msg_pl->sg.size; > if (unlikely((!split && >
Song Liu wrote: > On Wed, Jan 8, 2020 at 1:17 PM John Fastabend <john.fastabend@gmail.com> wrote: > > > > When passed a zero length skmsg tls_push_record() causes a NULL ptr > > deref. To resolve for fixes do a simple length check at start of > > routine. > > Could you please include the stack dump for the NULL deref? > > Thanks, > Song Sure I'll send a v2 with the stack dump.
John Fastabend wrote: > Song Liu wrote: > > On Wed, Jan 8, 2020 at 1:17 PM John Fastabend <john.fastabend@gmail.com> wrote: > > > > > > When passed a zero length skmsg tls_push_record() causes a NULL ptr > > > deref. To resolve for fixes do a simple length check at start of > > > routine. > > > > Could you please include the stack dump for the NULL deref? > > > > Thanks, > > Song > > Sure I'll send a v2 with the stack dump. Hi Song, I'm having a bit of trouble reproducing this now. I'm going to drop this patch from the series for now and see if something changed in crypto layers or elsewhere. I originally saw this on a bit older kernel so something might changed. Feels a bit like a work-around anyways so I'll dig into it a bit more. Either way I'll try and understand this a bit better. Thanks for the question.
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 21c7725d17ca..0326e916ab01 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -680,6 +680,9 @@ static int tls_push_record(struct sock *sk, int flags, msg_pl = &rec->msg_plaintext; msg_en = &rec->msg_encrypted; + if (unlikely(!msg_pl->sg.size)) + return 0; + split_point = msg_pl->apply_bytes; split = split_point && split_point < msg_pl->sg.size; if (unlikely((!split &&
When passed a zero length skmsg tls_push_record() causes a NULL ptr deref. To resolve for fixes do a simple length check at start of routine. To create this case a user can create a BPF program to pop all the data off the message then return SK_PASS. Its not a very practical or useful thing to do so we mark it unlikely. Fixes: d3b18ad31f93d ("tls: add bpf support to sk_msg handling") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- net/tls/tls_sw.c | 3 +++ 1 file changed, 3 insertions(+)