[bug report] Bluetooth: RFCOMM: Replace use of memcpy_from_msg with bt_skb_sendmmsg

[bug report] Bluetooth: RFCOMM: Replace use of memcpy_from_msg with bt_skb_sendmmsg

[Dan Carpenter]

Hello Luiz Augusto von Dentz,

The patch 81be03e026dc: "Bluetooth: RFCOMM: Replace use of
memcpy_from_msg with bt_skb_sendmmsg" from Sep 3, 2021, leads to the
following
Smatch static checker warning:

	net/bluetooth/rfcomm/sock.c:587 rfcomm_sock_sendmsg()
	warn: passing zero to 'PTR_ERR'

net/bluetooth/rfcomm/sock.c
    556 static int rfcomm_sock_sendmsg(struct socket *sock, struct msghdr *msg,
    557                                size_t len)
    558 {
    559         struct sock *sk = sock->sk;
    560         struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
    561         struct sk_buff *skb;
    562         int sent;
    563 
    564         if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
    565                 return -ENOTCONN;
    566 
    567         if (msg->msg_flags & MSG_OOB)
    568                 return -EOPNOTSUPP;
    569 
    570         if (sk->sk_shutdown & SEND_SHUTDOWN)
    571                 return -EPIPE;
    572 
    573         BT_DBG("sock %p, sk %p", sock, sk);
    574 
    575         lock_sock(sk);
    576 
    577         sent = bt_sock_wait_ready(sk, msg->msg_flags);
    578 
    579         release_sock(sk);
    580 
    581         if (sent)
    582                 return sent;
    583 
    584         skb = bt_skb_sendmmsg(sk, msg, len, d->mtu, RFCOMM_SKB_HEAD_RESERVE,
    585                               RFCOMM_SKB_TAIL_RESERVE);
    586         if (IS_ERR_OR_NULL(skb))

When a function returns both error pointers and NULL then that means
the feature is optional and can be turned off by the user.

	blinking_lights = get_blinking_lights();

We should report the error to the user.

	if (IS_ERR(blinking_lights))
		return PTR_ERR(blinking_lights);

However, some users maybe want a smaller kernel with no blinking lights
so they disable it.  In that case the driver has to check for NULL, and
not print an error message but instead continue as best as possible
without that feature enabled.

The bt_skb_sendmmsg() cannot return NULL.  But if it did return NULL
then PTR_ERR(NULL) is success so that's not right...  All the callers
of bt_skb_sendmmsg() have the same issue.

--> 587                 return PTR_ERR(skb);
    588 
    589         sent = rfcomm_dlc_send(d, skb);
    590         if (sent < 0)
    591                 kfree_skb(skb);
    592 
    593         return sent;
    594 }

regards,
dan carpenter

Re: [bug report] Bluetooth: RFCOMM: Replace use of memcpy_from_msg with bt_skb_sendmmsg

[Luiz Augusto von Dentz]

Hi Dan,

On Wed, Sep 15, 2021 at 2:27 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Luiz Augusto von Dentz,
>
> The patch 81be03e026dc: "Bluetooth: RFCOMM: Replace use of
> memcpy_from_msg with bt_skb_sendmmsg" from Sep 3, 2021, leads to the
> following
> Smatch static checker warning:
>
>         net/bluetooth/rfcomm/sock.c:587 rfcomm_sock_sendmsg()
>         warn: passing zero to 'PTR_ERR'
>
> net/bluetooth/rfcomm/sock.c
>     556 static int rfcomm_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>     557                                size_t len)
>     558 {
>     559         struct sock *sk = sock->sk;
>     560         struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
>     561         struct sk_buff *skb;
>     562         int sent;
>     563
>     564         if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
>     565                 return -ENOTCONN;
>     566
>     567         if (msg->msg_flags & MSG_OOB)
>     568                 return -EOPNOTSUPP;
>     569
>     570         if (sk->sk_shutdown & SEND_SHUTDOWN)
>     571                 return -EPIPE;
>     572
>     573         BT_DBG("sock %p, sk %p", sock, sk);
>     574
>     575         lock_sock(sk);
>     576
>     577         sent = bt_sock_wait_ready(sk, msg->msg_flags);
>     578
>     579         release_sock(sk);
>     580
>     581         if (sent)
>     582                 return sent;
>     583
>     584         skb = bt_skb_sendmmsg(sk, msg, len, d->mtu, RFCOMM_SKB_HEAD_RESERVE,
>     585                               RFCOMM_SKB_TAIL_RESERVE);
>     586         if (IS_ERR_OR_NULL(skb))
>
> When a function returns both error pointers and NULL then that means
> the feature is optional and can be turned off by the user.
>
>         blinking_lights = get_blinking_lights();
>
> We should report the error to the user.
>
>         if (IS_ERR(blinking_lights))
>                 return PTR_ERR(blinking_lights);
>
> However, some users maybe want a smaller kernel with no blinking lights
> so they disable it.  In that case the driver has to check for NULL, and
> not print an error message but instead continue as best as possible
> without that feature enabled.
>
> The bt_skb_sendmmsg() cannot return NULL.  But if it did return NULL
> then PTR_ERR(NULL) is success so that's not right...  All the callers
> of bt_skb_sendmmsg() have the same issue.

It should never be NULL though:

skb = bt_skb_send_alloc(sk, size + headroom + tailroom,
msg->msg_flags & MSG_DONTWAIT, &err);
if (!skb)
return ERR_PTR(err);

So I guess using IS_ERR_OR_NULL is misleading since we use the err
that comes from sock_alloc_send_skb so we might as well replace that
with IS_ERR instead.

> --> 587                 return PTR_ERR(skb);
>     588
>     589         sent = rfcomm_dlc_send(d, skb);
>     590         if (sent < 0)
>     591                 kfree_skb(skb);
>     592
>     593         return sent;
>     594 }
>
> regards,
> dan carpenter



-- 
Luiz Augusto von Dentz