netdev - Re: WARNING in isotp_tx_timer_handler and WARNING in print

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZCAytf0CpfAhjUSe@dragonet>
Date:   Sun, 26 Mar 2023 20:55:33 +0900
From:   "Dae R. Jeong" <threeearcat@...il.com>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     mkl@...gutronix.de, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 9bc344851704..0b95c0df7a63 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -912,13 +912,12 @@ static enum hrtimer_restart
> isotp_txfr_timer_handler(struct hrtimer *hrtimer)
>  		isotp_send_cframe(so);
> 
>  	return HRTIMER_NORESTART;
>  }
> 
> -static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t
> size)
> +static int isotp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t
> size)
>  {
> -	struct sock *sk = sock->sk;
>  	struct isotp_sock *so = isotp_sk(sk);
>  	u32 old_state = so->tx.state;
>  	struct sk_buff *skb;
>  	struct net_device *dev;
>  	struct canfd_frame *cf;
> @@ -1091,10 +1090,22 @@ static int isotp_sendmsg(struct socket *sock, struct
> msghdr *msg, size_t size)
>  		wake_up_interruptible(&so->wait);
> 
>  	return err;
>  }
> 
> +static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t
> size)
> +{
> +	struct sock *sk = sock->sk;
> +	int ret;
> +
> +	lock_sock(sk);
> +	ret = isotp_sendmsg_locked(sk, msg, size);
> +	release_sock(sk);
> +
> +	return ret;
> +}
> +
>  static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t
> size,
>  			 int flags)
>  {
>  	struct sock *sk = sock->sk;
>  	struct sk_buff *skb;

Hi, Oliver.

It seems that the patch should address the scenario I was thinking
of. But using a lock is always scary for a newbie like me because of
the possibility of causing other problems, e.g., deadlock. If it does
not cause other problems, it looks good for me.

Or although I'm not sure about this, what about getting rid of
reverting so->tx.state to old_state?

I think the concurrent execution of isotp_sendmsg() would be
problematic when reverting so->tx.state to old_state after goto'ing
err_out. There are two locations of "goto err_out", and
iostp_sendmsg() does nothing to the socket before both of "goto
err_out". So after goto'ing err_out, it seems fine for me even if we
do not revert so->tx.state to old_state.

If I think correctly, this will make cmpxchg() work, and prevent the
problematic concurrent execution. Could you please check the patch
below?

Best regards,
Dae R. Jeong.

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 9bc344851704..4630fad13803 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -918,7 +918,6 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 {
 	struct sock *sk = sock->sk;
 	struct isotp_sock *so = isotp_sk(sk);
-	u32 old_state = so->tx.state;
 	struct sk_buff *skb;
 	struct net_device *dev;
 	struct canfd_frame *cf;
@@ -1084,9 +1083,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 err_out_drop:
 	/* drop this PDU and unlock a potential wait queue */
-	old_state = ISOTP_IDLE;
+	so->tx.state = ISOTP_IDLE;
 err_out:
-	so->tx.state = old_state;
 	if (so->tx.state == ISOTP_IDLE)
 		wake_up_interruptible(&so->wait);