Re: [PATCH bpf] bpf: properly check TCP_CONGESTION optlen

[Daniel Borkmann <daniel@iogearbox.net>]

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