APPLIED/cmnt: [Trusty] [PATCH 2/2] Bluetooth: Check L2CAP option sizes returned from l2cap_get_conf_opt
Kleber Souza
kleber.souza at canonical.com
Tue Mar 12 11:43:58 UTC 2019
On 2/19/19 1:27 PM, 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)
> Signed-off-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
Applied to trusty/master-next branch, removing "linux-next" from the
provenance live above.
Thanks,
Kleber
> ---
> 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;
> }
More information about the kernel-team
mailing list