[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:16:00 UTC 2019
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.
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:
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