[Trusty] [PATCH 2/2] Bluetooth: Check L2CAP option sizes returned from l2cap_get_conf_opt
Tyler Hicks
tyhicks at canonical.com
Wed Feb 20 08:37:37 UTC 2019
On 2019-02-20 09:36:29, Kai Heng Feng wrote:
>
>
> > On Feb 20, 2019, at 9:16 AM, Tyler Hicks <tyhicks at canonical.com> wrote:
> >
> > On 2019-02-19 20:27:46, Kai-Heng Feng wrote:
> >> From: Marcel Holtmann <marcel at holtmann.org>
> >>
> >> When doing option parsing for standard type values of 1, 2 or 4 octets,
> >> the value is converted directly into a variable instead of a pointer. To
> >> avoid being tricked into being a pointer, check that for these option
> >> types that sizes actually match. In L2CAP every option is fixed size and
> >> thus it is prudent anyway to ensure that the remote side sends us the
> >> right option size along with option paramters.
> >>
> >> If the option size is not matching the option type, then that option is
> >> silently ignored. It is a protocol violation and instead of trying to
> >> give the remote attacker any further hints just pretend that option is
> >> not present and proceed with the default values. Implementation
> >> following the specification and its qualification procedures will always
> >> use the correct size and thus not being impacted here.
> >>
> >> To keep the code readable and consistent accross all options, a few
> >> cosmetic changes were also required.
> >>
> >> CVE-2019-3460
> >>
> >> Signed-off-by: Marcel Holtmann <marcel at holtmann.org>
> >> Reviewed-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> >> Signed-off-by: Johan Hedberg <johan.hedberg at intel.com>
> >> (backported from commit af3d5d1c87664a4f150fcf3534c6567cb19909b0 linux-next)
> >
> > Could you give a little info about what kind of backporting effort this
> > required? I quickly compared this patch to the upstream patch and it
> > mostly looked like contextual changes but I'd like for you to confirm
> > that.
>
> Yes, it’s just contextual change.
>
> >
> > Best practice would be to leave a little comment above your
> > Signed-off-by line. You're probably already aware of this but here's the
> > in-tree documentation on this practice:
>
> I’ve never added bracket comments in the past, but this is a good practice.
> I’ll do this in the future.
Thanks!
>
> Do you want me to re-send a V2?
No, I don't think there's a need for a v2.
Tyler
>
> Kai-Heng
>
> >
> > If you are a subsystem or branch maintainer, sometimes you need to
> > slightly modify patches you receive in order to merge them, because the
> > code is not exactly the same in your tree and the submitters’. If you
> > stick strictly to rule (c), you should ask the submitter to rediff, but
> > this is a totally counter-productive waste of time and energy. Rule (b)
> > allows you to adjust the code, but then it is very impolite to change
> > one submitter’s code and make him endorse your bugs. To solve this
> > problem, it is recommended that you add a line between the last
> > Signed-off-by header and yours, indicating the nature of your changes.
> > While there is nothing mandatory about this, it seems like prepending
> > the description with your mail and/or name, all enclosed in square
> > brackets, is noticeable enough to make it obvious that you are
> > responsible for last-minute changes. Example:
> >
> > Signed-off-by: Random J Developer <random at developer.example.org>
> > [lucky at maintainer.example.org: struct foo moved from foo.c to foo.h]
> > Signed-off-by: Lucky K Maintainer <lucky at maintainer.example.org>
> >
> > This practice is particularly helpful if you maintain a stable branch
> > and want at the same time to credit the author, track changes, merge the
> > fix, and protect the submitter from complaints. Note that under no
> > circumstances can you change the author’s identity (the From header), as
> > it is the one which appears in the changelog.
> >
> > - https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> >
> > Tyler
> >
> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
> >> ---
> >> net/bluetooth/l2cap_core.c | 77 +++++++++++++++++++++++---------------
> >> 1 file changed, 46 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >> index 61d0f290c0c6..a7de6cbdeced 100644
> >> --- a/net/bluetooth/l2cap_core.c
> >> +++ b/net/bluetooth/l2cap_core.c
> >> @@ -3284,10 +3284,14 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
> >>
> >> switch (type) {
> >> case L2CAP_CONF_MTU:
> >> + if (olen != 2)
> >> + break;
> >> mtu = val;
> >> break;
> >>
> >> case L2CAP_CONF_FLUSH_TO:
> >> + if (olen != 2)
> >> + break;
> >> chan->flush_to = val;
> >> break;
> >>
> >> @@ -3295,26 +3299,30 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
> >> break;
> >>
> >> case L2CAP_CONF_RFC:
> >> - if (olen == sizeof(rfc))
> >> - memcpy(&rfc, (void *) val, olen);
> >> + if (olen != sizeof(rfc))
> >> + break;
> >> + memcpy(&rfc, (void *) val, olen);
> >> break;
> >>
> >> case L2CAP_CONF_FCS:
> >> + if (olen != 1)
> >> + break;
> >> if (val == L2CAP_FCS_NONE)
> >> set_bit(CONF_RECV_NO_FCS, &chan->conf_state);
> >> break;
> >>
> >> case L2CAP_CONF_EFS:
> >> - if (olen == sizeof(efs)) {
> >> - remote_efs = 1;
> >> - memcpy(&efs, (void *) val, olen);
> >> - }
> >> + if (olen != sizeof(efs))
> >> + break;
> >> + remote_efs = 1;
> >> + memcpy(&efs, (void *) val, olen);
> >> break;
> >>
> >> case L2CAP_CONF_EWS:
> >> + if (olen != 2)
> >> + break;
> >> if (!chan->conn->hs_enabled)
> >> return -ECONNREFUSED;
> >> -
> >> set_bit(FLAG_EXT_CTRL, &chan->flags);
> >> set_bit(CONF_EWS_RECV, &chan->conf_state);
> >> chan->tx_win_max = L2CAP_DEFAULT_EXT_WINDOW;
> >> @@ -3324,7 +3332,6 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
> >> default:
> >> if (hint)
> >> break;
> >> -
> >> result = L2CAP_CONF_UNKNOWN;
> >> *((u8 *) ptr++) = type;
> >> break;
> >> @@ -3492,55 +3499,60 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
> >>
> >> switch (type) {
> >> case L2CAP_CONF_MTU:
> >> + if (olen != 2)
> >> + break;
> >> if (val < L2CAP_DEFAULT_MIN_MTU) {
> >> *result = L2CAP_CONF_UNACCEPT;
> >> chan->imtu = L2CAP_DEFAULT_MIN_MTU;
> >> } else
> >> chan->imtu = val;
> >> - l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu, endptr - ptr);
> >> + l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu,
> >> + endptr - ptr);
> >> break;
> >>
> >> case L2CAP_CONF_FLUSH_TO:
> >> + if (olen != 2)
> >> + break;
> >> chan->flush_to = val;
> >> - l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO,
> >> - 2, chan->flush_to, endptr - ptr);
> >> + l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO, 2,
> >> + chan->flush_to, endptr - ptr);
> >> break;
> >>
> >> case L2CAP_CONF_RFC:
> >> - if (olen == sizeof(rfc))
> >> - memcpy(&rfc, (void *)val, olen);
> >> -
> >> + if (olen != sizeof(rfc))
> >> + break;
> >> + memcpy(&rfc, (void *)val, olen);
> >> if (test_bit(CONF_STATE2_DEVICE, &chan->conf_state) &&
> >> rfc.mode != chan->mode)
> >> return -ECONNREFUSED;
> >> -
> >> chan->fcs = 0;
> >> -
> >> - l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC,
> >> - sizeof(rfc), (unsigned long) &rfc, endptr - ptr);
> >> + l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
> >> + (unsigned long) &rfc, endptr - ptr);
> >> break;
> >>
> >> case L2CAP_CONF_EWS:
> >> + if (olen != 2)
> >> + break;
> >> chan->ack_win = min_t(u16, val, chan->ack_win);
> >> l2cap_add_conf_opt(&ptr, L2CAP_CONF_EWS, 2,
> >> chan->tx_win, endptr - ptr);
> >> break;
> >>
> >> case L2CAP_CONF_EFS:
> >> - if (olen == sizeof(efs)) {
> >> - memcpy(&efs, (void *)val, olen);
> >> -
> >> - if (chan->local_stype != L2CAP_SERV_NOTRAFIC &&
> >> - efs.stype != L2CAP_SERV_NOTRAFIC &&
> >> - efs.stype != chan->local_stype)
> >> - return -ECONNREFUSED;
> >> -
> >> - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs),
> >> - (unsigned long) &efs, endptr - ptr);
> >> - }
> >> + if (olen != sizeof(efs))
> >> + break;
> >> + memcpy(&efs, (void *)val, olen);
> >> + if (chan->local_stype != L2CAP_SERV_NOTRAFIC &&
> >> + efs.stype != L2CAP_SERV_NOTRAFIC &&
> >> + efs.stype != chan->local_stype)
> >> + return -ECONNREFUSED;
> >> + l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs),
> >> + (unsigned long) &efs, endptr - ptr);
> >> break;
> >>
> >> case L2CAP_CONF_FCS:
> >> + if (olen != 1)
> >> + break;
> >> if (*result == L2CAP_CONF_PENDING)
> >> if (val == L2CAP_FCS_NONE)
> >> set_bit(CONF_RECV_NO_FCS,
> >> @@ -3655,10 +3667,13 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len)
> >>
> >> switch (type) {
> >> case L2CAP_CONF_RFC:
> >> - if (olen == sizeof(rfc))
> >> - memcpy(&rfc, (void *)val, olen);
> >> + if (olen != sizeof(rfc))
> >> + break;
> >> + memcpy(&rfc, (void *)val, olen);
> >> break;
> >> case L2CAP_CONF_EWS:
> >> + if (olen != 2)
> >> + break;
> >> txwin_ext = val;
> >> break;
> >> }
> >> --
> >> 2.17.1
> >>
> >>
> >> --
> >> kernel-team mailing list
> >> kernel-team at lists.ubuntu.com
> >> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
More information about the kernel-team
mailing list