On Fri, Jan 18, 2019 at 10:35:30AM +0100, Marcel Holtmann wrote:
> 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 :)
thanks,
greg k-h
