Re: [PATCH v1 1/1] Bluetooth: L2CAP: fix heap over-read in l2cap_get_conf_opt

[Paul Menzel]

Dear Muhammad,


Thank you for your patch.

Am 26.05.26 um 04:17 schrieb Muhammad Bilal:
> l2cap_get_conf_opt() reads opt->val via a switch on opt->len (1, 2,
> or 4 bytes).  opt->len is a remote-controlled u8.  All three callers
> loop on (len >= L2CAP_CONF_OPT_SIZE), so the loop body executes with
> as few as 2 bytes remaining.  A packet ending with opt->len=4 and
> only 2 bytes left causes get_unaligned_le32(opt->val) to read 4 bytes
> past the buffer before the caller can act on the return value.
>
> Commit 7c9cbd0b5e38 ("Bluetooth: Verify that l2cap_get_conf_opt
> provides large enough buffer") added a post-call len < 0 guard in
> each caller, but the over-read fires inside l2cap_get_conf_opt()
> before that guard is reached.
>
> Add a buflen parameter and validate L2CAP_CONF_OPT_SIZE + opt->len
> <= buflen before any access to opt->val.  Return -EINVAL on
> violation.  Update all three callers to capture the return value and
> break on negative.  With the bounds check ensuring the option fits
> within the remaining buffer, the post-call len < 0 check is no
> longer needed and is removed.

By any chance, do you have a reproducer?

> Fixes: 7c9cbd0b5e38 ("Bluetooth: Verify that l2cap_get_conf_opt provides large enough buffer")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Muhammad Bilal <meatuni001@xxxxxxxxx>
> ---
>   net/bluetooth/l2cap_core.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index fdccd62ccca8..6052ffb280ac 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3051,12 +3051,23 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn, u8 code,
>   }
>   
>   static inline int l2cap_get_conf_opt(void **ptr, int *type, int *olen,
> -				     unsigned long *val)
> +				     unsigned long *val, size_t buflen)
>   {
>   	struct l2cap_conf_opt *opt = *ptr;
>   	int len;
>   
> +	/* Guard opt->len dereference: reject if the 2-byte option header
> +	 * itself does not fit in the remaining buffer.
> +	 */
> +	if (buflen < L2CAP_CONF_OPT_SIZE)
> +		return -EINVAL;

I always wonder, if Linux should log a debug message or even warning.

> +
>   	len = L2CAP_CONF_OPT_SIZE + opt->len;
> +
> +	/* Reject options whose payload extends past the remaining buffer. */
> +	if ((size_t)len > buflen)
> +		return -EINVAL;

Ditto.

> +
>   	*ptr += len;
>   
>   	*type = opt->type;
> @@ -3437,9 +3448,11 @@ 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);
> -		if (len < 0)
> +		int optlen = l2cap_get_conf_opt(&req, &type, &olen, &val, len);
> +
> +		if (optlen < 0)
>   			break;
> +		len -= optlen;
>   
>   		hint  = type & L2CAP_CONF_HINT;
>   		type &= L2CAP_CONF_MASK;
> @@ -3675,9 +3688,11 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
>   	BT_DBG("chan %p, rsp %p, len %d, req %p", chan, rsp, len, data);
>   
>   	while (len >= L2CAP_CONF_OPT_SIZE) {
> -		len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
> -		if (len < 0)
> +		int optlen = l2cap_get_conf_opt(&rsp, &type, &olen, &val, len);
> +
> +		if (optlen < 0)
>   			break;
> +		len -= optlen;
>   
>   		switch (type) {
>   		case L2CAP_CONF_MTU:
> @@ -3946,9 +3961,11 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len)
>   		return;
>   
>   	while (len >= L2CAP_CONF_OPT_SIZE) {
> -		len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
> -		if (len < 0)
> +		int optlen = l2cap_get_conf_opt(&rsp, &type, &olen, &val, len);
> +
> +		if (optlen < 0)
>   			break;
> +		len -= optlen;
>   
>   		switch (type) {
>   		case L2CAP_CONF_RFC:

The diff looks good.

Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>


Kind regards,

Paul