Hi Greg,
>>> l2cap_get_conf_opt can handle a "default" message type, but it needs to
>>> be verified that it really is the correct type (CONF_EFS or CONF_RFC)
>>> before passing it back to the caller. To do this we need to check the
>>> return value of this call now and handle the error correctly up the
>>> stack.
>>>
>>> Based on a patch from Ran Menscher.
>>>
>>> Reported-by: Ran Menscher <ran.mensc...@karambasecurity.com>
>>> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
>>> ---
>>> net/bluetooth/l2cap_core.c | 25 +++++++++++++++++++------
>>> 1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index 2a7fb517d460..93daf94565cf 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -2980,6 +2980,10 @@ static inline int l2cap_get_conf_opt(void **ptr, int
>>> *type, int *olen,
>>> break;
>>>
>>> default:
>>> + /* Only CONF_EFS and CONF_RFC are allowed here */
>>> + if ((opt->type != L2CAP_CONF_EFS) &&
>>> + (opt->type != L2CAP_CONF_RFC))
>>> + return -EPROTO;
>>
>> after re-reading that specification, this also includes CONF_QOS since that
>> is a multi-field variable as well. Even if we currently don’t act on that
>> field, we need to accept it being send.
>
> /me hands you some \n characters...
>
> Ok, will fix up.
>
>>> *val = (unsigned long) opt->val;
>>> break;
>>> }
>>> @@ -3324,7 +3328,7 @@ static int l2cap_parse_conf_req(struct l2cap_chan
>>> *chan, void *data, size_t data
>>> void *endptr = data + data_size;
>>> void *req = chan->conf_req;
>>> int len = chan->conf_len;
>>> - int type, hint, olen;
>>> + int type, hint, olen, err;
>>> unsigned long val;
>>> struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC };
>>> struct l2cap_conf_efs efs;
>>> @@ -3336,7 +3340,10 @@ static int l2cap_parse_conf_req(struct l2cap_chan
>>> *chan, void *data, size_t data
>>> BT_DBG("chan %p", chan);
>>>
>>> while (len >= L2CAP_CONF_OPT_SIZE) {
>>> - len -= l2cap_get_conf_opt(&req, &type, &olen, &val);
>>> + err = l2cap_get_conf_opt(&req, &type, &olen, &val);
>>> + if (err < 0)
>>> + return err;
>>> + len -= err;
>>
>> We need to handle not yet known options correctly since otherwise we are
>> breaking forwards compatibility if newer specifications introduce new
>> parameters. So just returning with an error here is not acceptable. It will
>> fail qualification test cases.
>
> So what should we do here? We can't keep going as the size is
> incorrect.
>
>> Don’t we rather have proper length checks in l2cap_parse_conf_{req,rsp}
>> instead of doing this. I think your second patch is enough.
>
> It is? Ok, if that's all that is needed, that's fine with me. I was
> just taking the patch from the original submitter, I don't understand
> the bluetooth protocol at all :)
I need to get my brain back into the nasty details of that protocol. I know for
sure that just aborting is violating the handling of unknown options and that
will fail qualification.
Let me look at how they managed to trick us.
Regards
Marcel
