* [PATCH bpf] bpf: properly check TCP_CONGESTION optlen
@ 2019-02-23 19:07 Eric Dumazet
2019-02-23 20:38 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2019-02-23 19:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, netdev, Eric Dumazet, Eric Dumazet,
Martin KaFai Lau, Song Liu, Yonghong Song, bpf, Lawrence Brakmo
If caller of bpf_setsockopt() is silly passing a negative optlen
bad things happen.
Fixes: 91b5b21c7c16 ("bpf: Add support for changing congestion control")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Lawrence Brakmo <brakmo@fb.com>
---
net/core/filter.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index f7d0004fc16096eb42ece3a6acf645540ee2326b..6a5d89464168f2f35f43986c1dbc0446c9390a3c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4194,8 +4194,9 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
char name[TCP_CA_NAME_MAX];
bool reinit = bpf_sock->op > BPF_SOCK_OPS_NEEDS_ECN;
- strncpy(name, optval, min_t(long, optlen,
- TCP_CA_NAME_MAX-1));
+ if (optlen < 0)
+ return -EINVAL;
+ strncpy(name, optval, min(optlen, TCP_CA_NAME_MAX - 1));
name[TCP_CA_NAME_MAX-1] = 0;
ret = tcp_set_congestion_control(sk, name, false,
reinit);
--
2.21.0.rc0.258.g878e2cd30e-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: properly check TCP_CONGESTION optlen
2019-02-23 19:07 [PATCH bpf] bpf: properly check TCP_CONGESTION optlen Eric Dumazet
@ 2019-02-23 20:38 ` Alexei Starovoitov
2019-02-23 20:48 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2019-02-23 20:38 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Eric Dumazet,
Martin KaFai Lau, Song Liu, Yonghong Song, bpf, Lawrence Brakmo
On Sat, Feb 23, 2019 at 11:07:09AM -0800, Eric Dumazet wrote:
> If caller of bpf_setsockopt() is silly passing a negative optlen
> bad things happen.
>
> Fixes: 91b5b21c7c16 ("bpf: Add support for changing congestion control")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Lawrence Brakmo <brakmo@fb.com>
> ---
> net/core/filter.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index f7d0004fc16096eb42ece3a6acf645540ee2326b..6a5d89464168f2f35f43986c1dbc0446c9390a3c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4194,8 +4194,9 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
> char name[TCP_CA_NAME_MAX];
> bool reinit = bpf_sock->op > BPF_SOCK_OPS_NEEDS_ECN;
>
> - strncpy(name, optval, min_t(long, optlen,
> - TCP_CA_NAME_MAX-1));
> + if (optlen < 0)
> + return -EINVAL;
> + strncpy(name, optval, min(optlen, TCP_CA_NAME_MAX - 1));
Unnecessary.
The verifier guarantees that optlen > 0 because
static const struct bpf_func_proto bpf_setsockopt_proto = {
.func = bpf_setsockopt,
...
.arg5_type = ARG_CONST_SIZE,
};
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: properly check TCP_CONGESTION optlen
2019-02-23 20:38 ` Alexei Starovoitov
@ 2019-02-23 20:48 ` Eric Dumazet
2019-02-23 23:11 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2019-02-23 20:48 UTC (permalink / raw)
To: Alexei Starovoitov, Eric Dumazet
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Martin KaFai Lau,
Song Liu, Yonghong Song, bpf, Lawrence Brakmo
On 02/23/2019 12:38 PM, Alexei Starovoitov wrote:
> On Sat, Feb 23, 2019 at 11:07:09AM -0800, Eric Dumazet wrote:
>> If caller of bpf_setsockopt() is silly passing a negative optlen
>> bad things happen.
>>
>> Fixes: 91b5b21c7c16 ("bpf: Add support for changing congestion control")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Cc: Lawrence Brakmo <brakmo@fb.com>
>> ---
>> net/core/filter.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index f7d0004fc16096eb42ece3a6acf645540ee2326b..6a5d89464168f2f35f43986c1dbc0446c9390a3c 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -4194,8 +4194,9 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>> char name[TCP_CA_NAME_MAX];
>> bool reinit = bpf_sock->op > BPF_SOCK_OPS_NEEDS_ECN;
>>
>> - strncpy(name, optval, min_t(long, optlen,
>> - TCP_CA_NAME_MAX-1));
>> + if (optlen < 0)
>> + return -EINVAL;
>> + strncpy(name, optval, min(optlen, TCP_CA_NAME_MAX - 1));
>
> Unnecessary.
> The verifier guarantees that optlen > 0 because
> static const struct bpf_func_proto bpf_setsockopt_proto = {
> .func = bpf_setsockopt,
> ...
> .arg5_type = ARG_CONST_SIZE,
> };
>
Even on 32bit kernel ?
The suspect thing to me is the min_t(long, ....)
optlen is an integer, why do we need to promote to a long ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: properly check TCP_CONGESTION optlen
2019-02-23 20:48 ` Eric Dumazet
@ 2019-02-23 23:11 ` Alexei Starovoitov
2019-02-25 11:07 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2019-02-23 23:11 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric Dumazet, Alexei Starovoitov, Daniel Borkmann, netdev,
Martin KaFai Lau, Song Liu, Yonghong Song, bpf, Lawrence Brakmo
On Sat, Feb 23, 2019 at 12:48:53PM -0800, Eric Dumazet wrote:
>
>
> On 02/23/2019 12:38 PM, Alexei Starovoitov wrote:
> > On Sat, Feb 23, 2019 at 11:07:09AM -0800, Eric Dumazet wrote:
> >> If caller of bpf_setsockopt() is silly passing a negative optlen
> >> bad things happen.
> >>
> >> Fixes: 91b5b21c7c16 ("bpf: Add support for changing congestion control")
> >> Signed-off-by: Eric Dumazet <edumazet@google.com>
> >> Cc: Lawrence Brakmo <brakmo@fb.com>
> >> ---
> >> net/core/filter.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/core/filter.c b/net/core/filter.c
> >> index f7d0004fc16096eb42ece3a6acf645540ee2326b..6a5d89464168f2f35f43986c1dbc0446c9390a3c 100644
> >> --- a/net/core/filter.c
> >> +++ b/net/core/filter.c
> >> @@ -4194,8 +4194,9 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
> >> char name[TCP_CA_NAME_MAX];
> >> bool reinit = bpf_sock->op > BPF_SOCK_OPS_NEEDS_ECN;
> >>
> >> - strncpy(name, optval, min_t(long, optlen,
> >> - TCP_CA_NAME_MAX-1));
> >> + if (optlen < 0)
> >> + return -EINVAL;
> >> + strncpy(name, optval, min(optlen, TCP_CA_NAME_MAX - 1));
> >
> > Unnecessary.
> > The verifier guarantees that optlen > 0 because
> > static const struct bpf_func_proto bpf_setsockopt_proto = {
> > .func = bpf_setsockopt,
> > ...
> > .arg5_type = ARG_CONST_SIZE,
> > };
> >
>
> Even on 32bit kernel ?
>
> The suspect thing to me is the min_t(long, ....)
>
> optlen is an integer, why do we need to promote to a long ?
I think the code is actually fine as-is.
I bet it was copy pasted from do_tcp_setsockopt
where similar min_t(long) is used to match strncpy_from_user() declaration.
Here min_t(long) or min_t(int) or min() doesn't matter.
I would keep it as-is to avoid noisy patches.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: properly check TCP_CONGESTION optlen
2019-02-23 23:11 ` Alexei Starovoitov
@ 2019-02-25 11:07 ` Daniel Borkmann
2019-02-25 15:43 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2019-02-25 11:07 UTC (permalink / raw)
To: Alexei Starovoitov, Eric Dumazet
Cc: Eric Dumazet, Alexei Starovoitov, netdev, Martin KaFai Lau,
Song Liu, Yonghong Song, bpf, Lawrence Brakmo
On 02/24/2019 12:11 AM, Alexei Starovoitov wrote:
> On Sat, Feb 23, 2019 at 12:48:53PM -0800, Eric Dumazet wrote:
>> On 02/23/2019 12:38 PM, Alexei Starovoitov wrote:
>>> On Sat, Feb 23, 2019 at 11:07:09AM -0800, Eric Dumazet wrote:
>>>> If caller of bpf_setsockopt() is silly passing a negative optlen
>>>> bad things happen.
>>>>
>>>> Fixes: 91b5b21c7c16 ("bpf: Add support for changing congestion control")
>>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>>> Cc: Lawrence Brakmo <brakmo@fb.com>
>>>> ---
>>>> net/core/filter.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index f7d0004fc16096eb42ece3a6acf645540ee2326b..6a5d89464168f2f35f43986c1dbc0446c9390a3c 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -4194,8 +4194,9 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>>>> char name[TCP_CA_NAME_MAX];
>>>> bool reinit = bpf_sock->op > BPF_SOCK_OPS_NEEDS_ECN;
>>>>
>>>> - strncpy(name, optval, min_t(long, optlen,
>>>> - TCP_CA_NAME_MAX-1));
>>>> + if (optlen < 0)
>>>> + return -EINVAL;
>>>> + strncpy(name, optval, min(optlen, TCP_CA_NAME_MAX - 1));
>>>
>>> Unnecessary.
>>> The verifier guarantees that optlen > 0 because
>>> static const struct bpf_func_proto bpf_setsockopt_proto = {
>>> .func = bpf_setsockopt,
>>> ...
>>> .arg5_type = ARG_CONST_SIZE,
>>> };
>>
>> Even on 32bit kernel ?
>>
>> The suspect thing to me is the min_t(long, ....)
>>
>> optlen is an integer, why do we need to promote to a long ?
>
> I think the code is actually fine as-is.
> I bet it was copy pasted from do_tcp_setsockopt
> where similar min_t(long) is used to match strncpy_from_user() declaration.
> Here min_t(long) or min_t(int) or min() doesn't matter.
> I would keep it as-is to avoid noisy patches.
Max allowed input from verifier should be BPF_MAX_VAR_SIZ which is 1 << 29,
but I totally agree that the bpf_setsockopt() and bpf_getsockopt() signature
should change into u32 optlen as it's just confusing otherwise, same with the
long in strncpy(). Agree with Alexei that this might have been a copy-paste
kind of thing. Lawrence can probably clarify best?
The same wrong assumption is in commit 1e215300f138 ("bpf: add TCP_SAVE_SYN/
TCP_SAVED_SYN options for bpf_(set|get)sockopt") which tests for optlen <= 0.
Neither can it be negative nor zero here due to ARG_CONST_SIZE. Given it's
also confusing others, cleanup might still be worth considering. Maybe Lawrence
can spin this into one of his next patches, if noone else gets to it first.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: properly check TCP_CONGESTION optlen
2019-02-25 11:07 ` Daniel Borkmann
@ 2019-02-25 15:43 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2019-02-25 15:43 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Eric Dumazet, Alexei Starovoitov, netdev,
Martin KaFai Lau, Song Liu, Yonghong Song, bpf, Lawrence Brakmo
On Mon, Feb 25, 2019 at 3:07 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/24/2019 12:11 AM, Alexei Starovoitov wrote:
> > On Sat, Feb 23, 2019 at 12:48:53PM -0800, Eric Dumazet wrote:
> >> On 02/23/2019 12:38 PM, Alexei Starovoitov wrote:
> >>> On Sat, Feb 23, 2019 at 11:07:09AM -0800, Eric Dumazet wrote:
> >>>> If caller of bpf_setsockopt() is silly passing a negative optlen
> >>>> bad things happen.
> >>>>
> >>>> Fixes: 91b5b21c7c16 ("bpf: Add support for changing congestion control")
> >>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
> >>>> Cc: Lawrence Brakmo <brakmo@fb.com>
> >>>> ---
> >>>> net/core/filter.c | 5 +++--
> >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/net/core/filter.c b/net/core/filter.c
> >>>> index f7d0004fc16096eb42ece3a6acf645540ee2326b..6a5d89464168f2f35f43986c1dbc0446c9390a3c 100644
> >>>> --- a/net/core/filter.c
> >>>> +++ b/net/core/filter.c
> >>>> @@ -4194,8 +4194,9 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
> >>>> char name[TCP_CA_NAME_MAX];
> >>>> bool reinit = bpf_sock->op > BPF_SOCK_OPS_NEEDS_ECN;
> >>>>
> >>>> - strncpy(name, optval, min_t(long, optlen,
> >>>> - TCP_CA_NAME_MAX-1));
> >>>> + if (optlen < 0)
> >>>> + return -EINVAL;
> >>>> + strncpy(name, optval, min(optlen, TCP_CA_NAME_MAX - 1));
> >>>
> >>> Unnecessary.
> >>> The verifier guarantees that optlen > 0 because
> >>> static const struct bpf_func_proto bpf_setsockopt_proto = {
> >>> .func = bpf_setsockopt,
> >>> ...
> >>> .arg5_type = ARG_CONST_SIZE,
> >>> };
> >>
> >> Even on 32bit kernel ?
> >>
> >> The suspect thing to me is the min_t(long, ....)
> >>
> >> optlen is an integer, why do we need to promote to a long ?
> >
> > I think the code is actually fine as-is.
> > I bet it was copy pasted from do_tcp_setsockopt
> > where similar min_t(long) is used to match strncpy_from_user() declaration.
> > Here min_t(long) or min_t(int) or min() doesn't matter.
> > I would keep it as-is to avoid noisy patches.
>
> Max allowed input from verifier should be BPF_MAX_VAR_SIZ which is 1 << 29,
> but I totally agree that the bpf_setsockopt() and bpf_getsockopt() signature
> should change into u32 optlen as it's just confusing otherwise, same with the
> long in strncpy(). Agree with Alexei that this might have been a copy-paste
> kind of thing. Lawrence can probably clarify best?
>
> The same wrong assumption is in commit 1e215300f138 ("bpf: add TCP_SAVE_SYN/
> TCP_SAVED_SYN options for bpf_(set|get)sockopt") which tests for optlen <= 0.
> Neither can it be negative nor zero here due to ARG_CONST_SIZE. Given it's
> also confusing others, cleanup might still be worth considering. Maybe Lawrence
> can spin this into one of his next patches, if noone else gets to it first.
Yes, and considering the err_clear: error path does
memset(optval, 0, optlen);
This is even more confusing to think that optlen could be negative or
out of bound ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-25 15:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-23 19:07 [PATCH bpf] bpf: properly check TCP_CONGESTION optlen Eric Dumazet
2019-02-23 20:38 ` Alexei Starovoitov
2019-02-23 20:48 ` Eric Dumazet
2019-02-23 23:11 ` Alexei Starovoitov
2019-02-25 11:07 ` Daniel Borkmann
2019-02-25 15:43 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.