* [PATCH net 1/4] can: j1939: j1939_tp_tx_dat_new(): fix out-of-bounds memory access
2023-04-05 9:24 [PATCH net 0/4] pull-request: can 2023-04-05 Marc Kleine-Budde
@ 2023-04-05 9:24 ` Marc Kleine-Budde
2023-04-06 0:30 ` patchwork-bot+netdevbpf
2023-04-05 9:24 ` [PATCH net 2/4] can: isotp: isotp_recvmsg(): use sock_recv_cmsgs() to get SOCK_RXQ_OVFL infos Marc Kleine-Budde
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-04-05 9:24 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oleksij Rempel, Shuangpeng Bai,
stable, Marc Kleine-Budde
From: Oleksij Rempel <o.rempel@pengutronix.de>
In the j1939_tp_tx_dat_new() function, an out-of-bounds memory access
could occur during the memcpy() operation if the size of skb->cb is
larger than the size of struct j1939_sk_buff_cb. This is because the
memcpy() operation uses the size of skb->cb, leading to a read beyond
the struct j1939_sk_buff_cb.
Updated the memcpy() operation to use the size of struct
j1939_sk_buff_cb instead of the size of skb->cb. This ensures that the
memcpy() operation only reads the memory within the bounds of struct
j1939_sk_buff_cb, preventing out-of-bounds memory access.
Additionally, add a BUILD_BUG_ON() to check that the size of skb->cb
is greater than or equal to the size of struct j1939_sk_buff_cb. This
ensures that the skb->cb buffer is large enough to hold the
j1939_sk_buff_cb structure.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Reported-by: Shuangpeng Bai <sjb7183@psu.edu>
Tested-by: Shuangpeng Bai <sjb7183@psu.edu>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Link: https://groups.google.com/g/syzkaller/c/G_LL-C3plRs/m/-8xCi6dCAgAJ
Link: https://lore.kernel.org/all/20230404073128.3173900-1-o.rempel@pengutronix.de
Cc: stable@vger.kernel.org
[mkl: rephrase commit message]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/j1939/transport.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index fb92c3609e17..fe3df23a2595 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -604,7 +604,10 @@ sk_buff *j1939_tp_tx_dat_new(struct j1939_priv *priv,
/* reserve CAN header */
skb_reserve(skb, offsetof(struct can_frame, data));
- memcpy(skb->cb, re_skcb, sizeof(skb->cb));
+ /* skb->cb must be large enough to hold a j1939_sk_buff_cb structure */
+ BUILD_BUG_ON(sizeof(skb->cb) < sizeof(*re_skcb));
+
+ memcpy(skb->cb, re_skcb, sizeof(*re_skcb));
skcb = j1939_skb_to_cb(skb);
if (swap_src_dst)
j1939_skbcb_swap(skcb);
base-commit: 3ce9345580974863c060fa32971537996a7b2d57
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 1/4] can: j1939: j1939_tp_tx_dat_new(): fix out-of-bounds memory access
2023-04-05 9:24 ` [PATCH net 1/4] can: j1939: j1939_tp_tx_dat_new(): fix out-of-bounds memory access Marc Kleine-Budde
@ 2023-04-06 0:30 ` patchwork-bot+netdevbpf
0 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-04-06 0:30 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: netdev, davem, kuba, linux-can, kernel, o.rempel, sjb7183, stable
Hello:
This series was applied to netdev/net.git (main)
by Marc Kleine-Budde <mkl@pengutronix.de>:
On Wed, 5 Apr 2023 11:24:41 +0200 you wrote:
> From: Oleksij Rempel <o.rempel@pengutronix.de>
>
> In the j1939_tp_tx_dat_new() function, an out-of-bounds memory access
> could occur during the memcpy() operation if the size of skb->cb is
> larger than the size of struct j1939_sk_buff_cb. This is because the
> memcpy() operation uses the size of skb->cb, leading to a read beyond
> the struct j1939_sk_buff_cb.
>
> [...]
Here is the summary with links:
- [net,1/4] can: j1939: j1939_tp_tx_dat_new(): fix out-of-bounds memory access
https://git.kernel.org/netdev/net/c/b45193cb4df5
- [net,2/4] can: isotp: isotp_recvmsg(): use sock_recv_cmsgs() to get SOCK_RXQ_OVFL infos
https://git.kernel.org/netdev/net/c/0145462fc802
- [net,3/4] can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events
https://git.kernel.org/netdev/net/c/79e19fa79cb5
- [net,4/4] can: isotp: fix race between isotp_sendsmg() and isotp_release()
https://git.kernel.org/netdev/net/c/051737439eae
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 2/4] can: isotp: isotp_recvmsg(): use sock_recv_cmsgs() to get SOCK_RXQ_OVFL infos
2023-04-05 9:24 [PATCH net 0/4] pull-request: can 2023-04-05 Marc Kleine-Budde
2023-04-05 9:24 ` [PATCH net 1/4] can: j1939: j1939_tp_tx_dat_new(): fix out-of-bounds memory access Marc Kleine-Budde
@ 2023-04-05 9:24 ` Marc Kleine-Budde
2023-04-05 9:24 ` [PATCH net 3/4] can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events Marc Kleine-Budde
2023-04-05 9:24 ` [PATCH net 4/4] can: isotp: fix race between isotp_sendsmg() and isotp_release() Marc Kleine-Budde
3 siblings, 0 replies; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-04-05 9:24 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oliver Hartkopp, stable,
Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
isotp.c was still using sock_recv_timestamp() which does not provide
control messages to detect dropped PDUs in the receive path.
Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://lore.kernel.org/all/20230330170248.62342-1-socketcan@hartkopp.net
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/isotp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 9bc344851704..47c2ebad10ed 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1120,7 +1120,7 @@ static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
if (ret < 0)
goto out_err;
- sock_recv_timestamp(msg, sk, skb);
+ sock_recv_cmsgs(msg, sk, skb);
if (msg->msg_name) {
__sockaddr_check_size(ISOTP_MIN_NAMELEN);
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net 3/4] can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events
2023-04-05 9:24 [PATCH net 0/4] pull-request: can 2023-04-05 Marc Kleine-Budde
2023-04-05 9:24 ` [PATCH net 1/4] can: j1939: j1939_tp_tx_dat_new(): fix out-of-bounds memory access Marc Kleine-Budde
2023-04-05 9:24 ` [PATCH net 2/4] can: isotp: isotp_recvmsg(): use sock_recv_cmsgs() to get SOCK_RXQ_OVFL infos Marc Kleine-Budde
@ 2023-04-05 9:24 ` Marc Kleine-Budde
2023-04-05 9:24 ` [PATCH net 4/4] can: isotp: fix race between isotp_sendsmg() and isotp_release() Marc Kleine-Budde
3 siblings, 0 replies; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-04-05 9:24 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Michal Sojka, Jakub Jira,
Oliver Hartkopp, stable, Marc Kleine-Budde
From: Michal Sojka <michal.sojka@cvut.cz>
When using select()/poll()/epoll() with a non-blocking ISOTP socket to
wait for when non-blocking write is possible, a false EPOLLOUT event
is sometimes returned. This can happen at least after sending a
message which must be split to multiple CAN frames.
The reason is that isotp_sendmsg() returns -EAGAIN when tx.state is
not equal to ISOTP_IDLE and this behavior is not reflected in
datagram_poll(), which is used in isotp_ops.
This is fixed by introducing ISOTP-specific poll function, which
suppresses the EPOLLOUT events in that case.
v2: https://lore.kernel.org/all/20230302092812.320643-1-michal.sojka@cvut.cz
v1: https://lore.kernel.org/all/20230224010659.48420-1-michal.sojka@cvut.cz
https://lore.kernel.org/all/b53a04a2-ba1f-3858-84c1-d3eb3301ae15@hartkopp.net
Signed-off-by: Michal Sojka <michal.sojka@cvut.cz>
Reported-by: Jakub Jira <jirajak2@fel.cvut.cz>
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Link: https://lore.kernel.org/all/20230331125511.372783-1-michal.sojka@cvut.cz
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/isotp.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 47c2ebad10ed..281b7766c54e 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1608,6 +1608,21 @@ static int isotp_init(struct sock *sk)
return 0;
}
+static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *wait)
+{
+ struct sock *sk = sock->sk;
+ struct isotp_sock *so = isotp_sk(sk);
+
+ __poll_t mask = datagram_poll(file, sock, wait);
+ poll_wait(file, &so->wait, wait);
+
+ /* Check for false positives due to TX state */
+ if ((mask & EPOLLWRNORM) && (so->tx.state != ISOTP_IDLE))
+ mask &= ~(EPOLLOUT | EPOLLWRNORM);
+
+ return mask;
+}
+
static int isotp_sock_no_ioctlcmd(struct socket *sock, unsigned int cmd,
unsigned long arg)
{
@@ -1623,7 +1638,7 @@ static const struct proto_ops isotp_ops = {
.socketpair = sock_no_socketpair,
.accept = sock_no_accept,
.getname = isotp_getname,
- .poll = datagram_poll,
+ .poll = isotp_poll,
.ioctl = isotp_sock_no_ioctlcmd,
.gettstamp = sock_gettstamp,
.listen = sock_no_listen,
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net 4/4] can: isotp: fix race between isotp_sendsmg() and isotp_release()
2023-04-05 9:24 [PATCH net 0/4] pull-request: can 2023-04-05 Marc Kleine-Budde
` (2 preceding siblings ...)
2023-04-05 9:24 ` [PATCH net 3/4] can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events Marc Kleine-Budde
@ 2023-04-05 9:24 ` Marc Kleine-Budde
3 siblings, 0 replies; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-04-05 9:24 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oliver Hartkopp, Dae R . Jeong,
Hillf Danton, stable, Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
As discussed with Dae R. Jeong and Hillf Danton here [1] the sendmsg()
function in isotp.c might get into a race condition when restoring the
former tx.state from the old_state.
Remove the old_state concept and implement proper locking for the
ISOTP_IDLE transitions in isotp_sendmsg(), inspired by a
simplification idea from Hillf Danton.
Introduce a new tx.state ISOTP_SHUTDOWN and use the same locking
mechanism from isotp_release() which resolves a potential race between
isotp_sendsmg() and isotp_release().
[1] https://lore.kernel.org/linux-can/ZB%2F93xJxq%2FBUqAgG@dragonet
v1: https://lore.kernel.org/all/20230331102114.15164-1-socketcan@hartkopp.net
v2: https://lore.kernel.org/all/20230331123600.3550-1-socketcan@hartkopp.net
take care of signal interrupts for wait_event_interruptible() in
isotp_release()
v3: https://lore.kernel.org/all/20230331130654.9886-1-socketcan@hartkopp.net
take care of signal interrupts for wait_event_interruptible() in
isotp_sendmsg() in the wait_tx_done case
v4: https://lore.kernel.org/all/20230331131935.21465-1-socketcan@hartkopp.net
take care of signal interrupts for wait_event_interruptible() in
isotp_sendmsg() in ALL cases
Cc: Dae R. Jeong <threeearcat@gmail.com>
Cc: Hillf Danton <hdanton@sina.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Fixes: 4f027cba8216 ("can: isotp: split tx timer into transmission and timeout")
Link: https://lore.kernel.org/all/20230331131935.21465-1-socketcan@hartkopp.net
Cc: stable@vger.kernel.org
[mkl: rephrase commit message]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/isotp.c | 55 ++++++++++++++++++++++++++++---------------------
1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 281b7766c54e..5761d4ab839d 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -119,7 +119,8 @@ enum {
ISOTP_WAIT_FIRST_FC,
ISOTP_WAIT_FC,
ISOTP_WAIT_DATA,
- ISOTP_SENDING
+ ISOTP_SENDING,
+ ISOTP_SHUTDOWN,
};
struct tpcon {
@@ -880,8 +881,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
txtimer);
struct sock *sk = &so->sk;
- /* don't handle timeouts in IDLE state */
- if (so->tx.state == ISOTP_IDLE)
+ /* don't handle timeouts in IDLE or SHUTDOWN state */
+ if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
return HRTIMER_NORESTART;
/* we did not get any flow control or echo frame in time */
@@ -918,7 +919,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;
@@ -928,23 +928,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
int off;
int err;
- if (!so->bound)
+ if (!so->bound || so->tx.state == ISOTP_SHUTDOWN)
return -EADDRNOTAVAIL;
+wait_free_buffer:
/* we do not support multiple buffers - for now */
- if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE ||
- wq_has_sleeper(&so->wait)) {
- if (msg->msg_flags & MSG_DONTWAIT) {
- err = -EAGAIN;
- goto err_out;
- }
+ if (wq_has_sleeper(&so->wait) && (msg->msg_flags & MSG_DONTWAIT))
+ return -EAGAIN;
- /* wait for complete transmission of current pdu */
- err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
- if (err)
- goto err_out;
+ /* wait for complete transmission of current pdu */
+ err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ if (err)
+ goto err_event_drop;
- so->tx.state = ISOTP_SENDING;
+ if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
+ if (so->tx.state == ISOTP_SHUTDOWN)
+ return -EADDRNOTAVAIL;
+
+ goto wait_free_buffer;
}
if (!size || size > MAX_MSG_LENGTH) {
@@ -1074,7 +1075,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
if (wait_tx_done) {
/* wait for complete transmission of current pdu */
- wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ if (err)
+ goto err_event_drop;
if (sk->sk_err)
return -sk->sk_err;
@@ -1082,13 +1085,15 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
return size;
+err_event_drop:
+ /* got signal: force tx state machine to be idle */
+ so->tx.state = ISOTP_IDLE;
+ hrtimer_cancel(&so->txfrtimer);
+ hrtimer_cancel(&so->txtimer);
err_out_drop:
/* drop this PDU and unlock a potential wait queue */
- old_state = ISOTP_IDLE;
-err_out:
- so->tx.state = old_state;
- if (so->tx.state == ISOTP_IDLE)
- wake_up_interruptible(&so->wait);
+ so->tx.state = ISOTP_IDLE;
+ wake_up_interruptible(&so->wait);
return err;
}
@@ -1150,10 +1155,12 @@ static int isotp_release(struct socket *sock)
net = sock_net(sk);
/* wait for complete transmission of current pdu */
- wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ while (wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE) == 0 &&
+ cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SHUTDOWN) != ISOTP_IDLE)
+ ;
/* force state machines to be idle also when a signal occurred */
- so->tx.state = ISOTP_IDLE;
+ so->tx.state = ISOTP_SHUTDOWN;
so->rx.state = ISOTP_IDLE;
spin_lock(&isotp_notifier_lock);
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread