The backport of commit 9c5df2f14ee3 ("can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events") introduced a new regression where the fix could potentially introduce new side effects.
To reduce the risk of other unmet dependencies and missing fixes and checks the latest 6.1 LTS code base is ported back to the 5.15 LTS tree.
Lukas Magel (1): can: isotp: isotp_sendmsg(): fix TX state detection and wait behavior
Oliver Hartkopp (6): can: isotp: set max PDU size to 64 kByte can: isotp: isotp_bind(): return -EINVAL on incorrect CAN ID formatting can: isotp: check CAN address family in isotp_bind() can: isotp: handle wait_event_interruptible() return values can: isotp: add local echo tx processing and tx without FC can: isotp: isotp_bind(): do not validate unused address information
include/uapi/linux/can/isotp.h | 25 +- net/can/isotp.c | 426 +++++++++++++++++++++------------ 2 files changed, 288 insertions(+), 163 deletions(-)
commit 9c0c191d82a1de964ac953a1df8b5744ec670b07 upstream
The reason to extend the max PDU size from 4095 Byte (12 bit length value) to a 32 bit value (up to 4 GByte) was to be able to flash 64 kByte bootloaders with a single ISO-TP PDU. The max PDU size in the Linux kernel implementation was set to 8200 Bytes to be able to test the length information escape sequence.
It turns out that the demand for 64 kByte PDUs is real so the value for MAX_MSG_LENGTH is set to 66000 to be able to potentially add some checksums to the 65.536 Byte block.
Link: https://github.com/linux-can/can-utils/issues/347#issuecomment-1056142301 Link: https://lore.kernel.org/all/20220309120416.83514-3-socketcan@hartkopp.net Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de --- net/can/isotp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c index 4dccf7b4b88d..3e2a7cbade24 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -85,13 +85,13 @@ MODULE_ALIAS("can-proto-6"); (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
/* ISO 15765-2:2016 supports more than 4095 byte per ISO PDU as the FF_DL can * take full 32 bit values (4 Gbyte). We would need some good concept to handle * this between user space and kernel space. For now increase the static buffer - * to something about 8 kbyte to be able to test this new functionality. + * to something about 64 kbyte to be able to test this new functionality. */ -#define MAX_MSG_LENGTH 8200 +#define MAX_MSG_LENGTH 66000
/* N_PCI type values in bits 7-4 of N_PCI bytes */ #define N_PCI_SF 0x00 /* single frame */ #define N_PCI_FF 0x10 /* first frame */ #define N_PCI_CF 0x20 /* consecutive frame */
commit 2aa39889c463195a0dfe2aff9fad413139c32a4f upstream
Commit 3ea566422cbd ("can: isotp: sanitize CAN ID checks in isotp_bind()") checks the given CAN ID address information by sanitizing the input values.
This check (silently) removes obsolete bits by masking the given CAN IDs.
Derek Will suggested to give a feedback to the application programmer when the 'sanitizing' was actually needed which means the programmer provided CAN ID content in a wrong format (e.g. SFF CAN IDs with a CAN ID > 0x7FF).
Link: https://lore.kernel.org/all/20220515181633.76671-1-socketcan@hartkopp.net Suggested-by: Derek Will derekrobertwill@gmail.com Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de --- net/can/isotp.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/net/can/isotp.c b/net/can/isotp.c index 3e2a7cbade24..b5fdf30a6996 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1140,10 +1140,15 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) if (rx_id & CAN_EFF_FLAG) rx_id &= (CAN_EFF_FLAG | CAN_EFF_MASK); else rx_id &= CAN_SFF_MASK;
+ /* give feedback on wrong CAN-ID values */ + if (tx_id != addr->can_addr.tp.tx_id || + rx_id != addr->can_addr.tp.rx_id) + return -EINVAL; + if (!addr->can_ifindex) return -ENODEV;
lock_sock(sk);
commit c6adf659a8ba85913e16a571d5a9bcd17d3d1234 upstream
Add missing check to block non-AF_CAN binds.
Syzbot created some code which matched the right sockaddr struct size but used AF_XDP (0x2C) instead of AF_CAN (0x1D) in the address family field:
bind$xdp(r2, &(0x7f0000000540)={0x2c, 0x0, r4, 0x0, r2}, 0x10) ^^^^ This has no funtional impact but the userspace should be notified about the wrong address family field content.
Link: https://syzkaller.appspot.com/text?tag=CrashLog&x=11ff9d8c480000 Reported-by: syzbot+5aed6c3aaba661f5b917@syzkaller.appspotmail.com Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net Link: https://lore.kernel.org/all/20230104201844.13168-1-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de --- net/can/isotp.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/net/can/isotp.c b/net/can/isotp.c index b5fdf30a6996..82e56f3dd2c6 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1127,10 +1127,13 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) int do_rx_reg = 1;
if (len < ISOTP_MIN_NAMELEN) return -EINVAL;
+ if (addr->can_family != AF_CAN) + return -EINVAL; + /* sanitize tx/rx CAN identifiers */ tx_id = addr->can_addr.tp.tx_id; if (tx_id & CAN_EFF_FLAG) tx_id &= (CAN_EFF_FLAG | CAN_EFF_MASK); else
commit 823b2e42720f96f277940c37ea438b7c5ead51a4 upstream
When wait_event_interruptible() has been interrupted by a signal the tx.state value might not be ISOTP_IDLE. Force the state machines into idle state to inhibit the timer handlers to continue working.
Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing") Cc: stable@vger.kernel.org Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net Link: https://lore.kernel.org/all/20230112192347.1944-1-socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de --- net/can/isotp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/net/can/isotp.c b/net/can/isotp.c index 82e56f3dd2c6..683010c71a8a 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1069,10 +1069,14 @@ 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);
+ /* force state machines to be idle also when a signal occurred */ + so->tx.state = ISOTP_IDLE; + so->rx.state = ISOTP_IDLE; + spin_lock(&isotp_notifier_lock); while (isotp_busy_notifier == so) { spin_unlock(&isotp_notifier_lock); schedule_timeout_uninterruptible(1); spin_lock(&isotp_notifier_lock);
commit 4b7fe92c06901f4563af0e36d25223a5ab343782 upstream commit 9f39d36530e5678d092d53c5c2c60d82b4dcc169 upstream commit 051737439eaee5bdd03d3c2ef5510d54a478fd05 upstream
Due to the existing patch order applied to isotp.c in the stable kernel the original order of depending patches the three original patches 4b7fe92c0690 ("can: isotp: add local echo tx processing for consecutive frames") 9f39d36530e5 ("can: isotp: add support for transmission without flow control") 051737439eae ("can: isotp: fix race between isotp_sendsmg() and isotp_release()") can not be split into different patches that can be applied in working steps to the stable tree.
Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net --- include/uapi/linux/can/isotp.h | 25 ++- net/can/isotp.c | 398 +++++++++++++++++++++------------ 2 files changed, 267 insertions(+), 156 deletions(-)
diff --git a/include/uapi/linux/can/isotp.h b/include/uapi/linux/can/isotp.h index 590f8aea2b6d..439c982f7e81 100644 --- a/include/uapi/linux/can/isotp.h +++ b/include/uapi/linux/can/isotp.h @@ -122,22 +122,23 @@ struct can_isotp_ll_options { /* by the CAN netdriver configuration */ };
/* flags for isotp behaviour */
-#define CAN_ISOTP_LISTEN_MODE 0x001 /* listen only (do not send FC) */ -#define CAN_ISOTP_EXTEND_ADDR 0x002 /* enable extended addressing */ -#define CAN_ISOTP_TX_PADDING 0x004 /* enable CAN frame padding tx path */ -#define CAN_ISOTP_RX_PADDING 0x008 /* enable CAN frame padding rx path */ -#define CAN_ISOTP_CHK_PAD_LEN 0x010 /* check received CAN frame padding */ -#define CAN_ISOTP_CHK_PAD_DATA 0x020 /* check received CAN frame padding */ -#define CAN_ISOTP_HALF_DUPLEX 0x040 /* half duplex error state handling */ -#define CAN_ISOTP_FORCE_TXSTMIN 0x080 /* ignore stmin from received FC */ -#define CAN_ISOTP_FORCE_RXSTMIN 0x100 /* ignore CFs depending on rx stmin */ -#define CAN_ISOTP_RX_EXT_ADDR 0x200 /* different rx extended addressing */ -#define CAN_ISOTP_WAIT_TX_DONE 0x400 /* wait for tx completion */ -#define CAN_ISOTP_SF_BROADCAST 0x800 /* 1-to-N functional addressing */ +#define CAN_ISOTP_LISTEN_MODE 0x0001 /* listen only (do not send FC) */ +#define CAN_ISOTP_EXTEND_ADDR 0x0002 /* enable extended addressing */ +#define CAN_ISOTP_TX_PADDING 0x0004 /* enable CAN frame padding tx path */ +#define CAN_ISOTP_RX_PADDING 0x0008 /* enable CAN frame padding rx path */ +#define CAN_ISOTP_CHK_PAD_LEN 0x0010 /* check received CAN frame padding */ +#define CAN_ISOTP_CHK_PAD_DATA 0x0020 /* check received CAN frame padding */ +#define CAN_ISOTP_HALF_DUPLEX 0x0040 /* half duplex error state handling */ +#define CAN_ISOTP_FORCE_TXSTMIN 0x0080 /* ignore stmin from received FC */ +#define CAN_ISOTP_FORCE_RXSTMIN 0x0100 /* ignore CFs depending on rx stmin */ +#define CAN_ISOTP_RX_EXT_ADDR 0x0200 /* different rx extended addressing */ +#define CAN_ISOTP_WAIT_TX_DONE 0x0400 /* wait for tx completion */ +#define CAN_ISOTP_SF_BROADCAST 0x0800 /* 1-to-N functional addressing */ +#define CAN_ISOTP_CF_BROADCAST 0x1000 /* 1-to-N transmission w/o FC */
/* protocol machine default values */
#define CAN_ISOTP_DEFAULT_FLAGS 0 #define CAN_ISOTP_DEFAULT_EXT_ADDRESS 0x00 diff --git a/net/can/isotp.c b/net/can/isotp.c index 683010c71a8a..19fffa8676f9 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -12,11 +12,10 @@ * - TX path flowcontrol reception with wrong layout/padding leads to -EBADMSG * - when a transfer (tx) is on the run the next write() blocks until it's done * - use CAN_ISOTP_WAIT_TX_DONE flag to block the caller until the PDU is sent * - as we have static buffers the check whether the PDU fits into the buffer * is done at FF reception time (no support for sending 'wait frames') - * - take care of the tx-queue-len as traffic shaping is still on the TODO list * * Copyright (c) 2020 Volkswagen Group Electronic Research * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -103,22 +102,27 @@ MODULE_ALIAS("can-proto-6"); #define FF_PCI_SZ12 2 /* size of FirstFrame PCI including 12 bit FF_DL */ #define FF_PCI_SZ32 6 /* size of FirstFrame PCI including 32 bit FF_DL */ #define FC_CONTENT_SZ 3 /* flow control content size in byte (FS/BS/STmin) */
#define ISOTP_CHECK_PADDING (CAN_ISOTP_CHK_PAD_LEN | CAN_ISOTP_CHK_PAD_DATA) +#define ISOTP_ALL_BC_FLAGS (CAN_ISOTP_SF_BROADCAST | CAN_ISOTP_CF_BROADCAST)
/* Flow Status given in FC frame */ #define ISOTP_FC_CTS 0 /* clear to send */ #define ISOTP_FC_WT 1 /* wait */ #define ISOTP_FC_OVFLW 2 /* overflow */
+#define ISOTP_FC_TIMEOUT 1 /* 1 sec */ +#define ISOTP_ECHO_TIMEOUT 2 /* 2 secs */ + enum { ISOTP_IDLE = 0, ISOTP_WAIT_FIRST_FC, ISOTP_WAIT_FC, ISOTP_WAIT_DATA, - ISOTP_SENDING + ISOTP_SENDING, + ISOTP_SHUTDOWN, };
struct tpcon { unsigned int idx; unsigned int len; @@ -135,17 +139,18 @@ struct isotp_sock { int ifindex; canid_t txid; canid_t rxid; ktime_t tx_gap; ktime_t lastrxcf_tstamp; - struct hrtimer rxtimer, txtimer; + struct hrtimer rxtimer, txtimer, txfrtimer; struct can_isotp_options opt; struct can_isotp_fc_options rxfc, txfc; struct can_isotp_ll_options ll; u32 frame_txtime; u32 force_tx_stmin; u32 force_rx_stmin; + u32 cfecho; /* consecutive frame echo tag */ struct tpcon rx, tx; struct list_head notifier; wait_queue_head_t wait; spinlock_t rx_lock; /* protect single thread state machine */ }; @@ -157,10 +162,21 @@ static struct isotp_sock *isotp_busy_notifier; static inline struct isotp_sock *isotp_sk(const struct sock *sk) { return (struct isotp_sock *)sk; }
+static u32 isotp_bc_flags(struct isotp_sock *so) +{ + return so->opt.flags & ISOTP_ALL_BC_FLAGS; +} + +static bool isotp_register_rxid(struct isotp_sock *so) +{ + /* no broadcast modes => register rx_id for FC frame reception */ + return (isotp_bc_flags(so) == 0); +} + static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer) { struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, rxtimer); struct sock *sk = &so->sk; @@ -238,11 +254,12 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
/* reset last CF frame rx timestamp for rx stmin enforcement */ so->lastrxcf_tstamp = ktime_set(0, 0);
/* start rx timeout watchdog */ - hrtimer_start(&so->rxtimer, ktime_set(1, 0), HRTIMER_MODE_REL_SOFT); + hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0), + HRTIMER_MODE_REL_SOFT); return 0; }
static void isotp_rcv_skb(struct sk_buff *skb, struct sock *sk) { @@ -324,10 +341,12 @@ static int check_pad(struct isotp_sock *so, struct canfd_frame *cf, return 1; } return 0; }
+static void isotp_send_cframe(struct isotp_sock *so); + static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae) { struct sock *sk = &so->sk;
if (so->tx.state != ISOTP_WAIT_FC && @@ -378,18 +397,19 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
switch (cf->data[ae] & 0x0F) { case ISOTP_FC_CTS: so->tx.bs = 0; so->tx.state = ISOTP_SENDING; - /* start cyclic timer for sending CF frame */ - hrtimer_start(&so->txtimer, so->tx_gap, + /* send CF frame and enable echo timeout handling */ + hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0), HRTIMER_MODE_REL_SOFT); + isotp_send_cframe(so); break;
case ISOTP_FC_WT: /* start timer to wait for next FC frame */ - hrtimer_start(&so->txtimer, ktime_set(1, 0), + hrtimer_start(&so->txtimer, ktime_set(ISOTP_FC_TIMEOUT, 0), HRTIMER_MODE_REL_SOFT); break;
case ISOTP_FC_OVFLW: /* overflow on receiver side - report 'message too long' */ @@ -580,11 +600,11 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae, }
/* perform blocksize handling, if enabled */ if (!so->rxfc.bs || ++so->rx.bs < so->rxfc.bs) { /* start rx timeout watchdog */ - hrtimer_start(&so->rxtimer, ktime_set(1, 0), + hrtimer_start(&so->rxtimer, ktime_set(ISOTP_FC_TIMEOUT, 0), HRTIMER_MODE_REL_SOFT); return 0; }
/* no creation of flow control frames */ @@ -711,10 +731,67 @@ static void isotp_fill_dataframe(struct canfd_frame *cf, struct isotp_sock *so,
if (ae) cf->data[0] = so->opt.ext_address; }
+static void isotp_send_cframe(struct isotp_sock *so) +{ + struct sock *sk = &so->sk; + struct sk_buff *skb; + struct net_device *dev; + struct canfd_frame *cf; + int can_send_ret; + int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0; + + dev = dev_get_by_index(sock_net(sk), so->ifindex); + if (!dev) + return; + + skb = alloc_skb(so->ll.mtu + sizeof(struct can_skb_priv), GFP_ATOMIC); + if (!skb) { + dev_put(dev); + return; + } + + can_skb_reserve(skb); + can_skb_prv(skb)->ifindex = dev->ifindex; + can_skb_prv(skb)->skbcnt = 0; + + cf = (struct canfd_frame *)skb->data; + skb_put_zero(skb, so->ll.mtu); + + /* create consecutive frame */ + isotp_fill_dataframe(cf, so, ae, 0); + + /* place consecutive frame N_PCI in appropriate index */ + cf->data[ae] = N_PCI_CF | so->tx.sn++; + so->tx.sn %= 16; + so->tx.bs++; + + cf->flags = so->ll.tx_flags; + + skb->dev = dev; + can_skb_set_owner(skb, sk); + + /* cfecho should have been zero'ed by init/isotp_rcv_echo() */ + if (so->cfecho) + pr_notice_once("can-isotp: cfecho is %08X != 0\n", so->cfecho); + + /* set consecutive frame echo tag */ + so->cfecho = *(u32 *)cf->data; + + /* send frame with local echo enabled */ + can_send_ret = can_send(skb, 1); + if (can_send_ret) { + pr_notice_once("can-isotp: %s: can_send_ret %pe\n", + __func__, ERR_PTR(can_send_ret)); + if (can_send_ret == -ENOBUFS) + pr_notice_once("can-isotp: tx queue is full\n"); + } + dev_put(dev); +} + static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so, int ae) { int i; int ff_pci_sz; @@ -744,149 +821,127 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so, /* add first data bytes depending on ae */ for (i = ae + ff_pci_sz; i < so->tx.ll_dl; i++) cf->data[i] = so->tx.buf[so->tx.idx++];
so->tx.sn = 1; - so->tx.state = ISOTP_WAIT_FIRST_FC; }
-static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer) +static void isotp_rcv_echo(struct sk_buff *skb, void *data) { - struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, - txtimer); - struct sock *sk = &so->sk; - struct sk_buff *skb; - struct net_device *dev; - struct canfd_frame *cf; - enum hrtimer_restart restart = HRTIMER_NORESTART; - int can_send_ret; - int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0; + struct sock *sk = (struct sock *)data; + struct isotp_sock *so = isotp_sk(sk); + struct canfd_frame *cf = (struct canfd_frame *)skb->data;
- switch (so->tx.state) { - case ISOTP_WAIT_FC: - case ISOTP_WAIT_FIRST_FC: + /* only handle my own local echo CF/SF skb's (no FF!) */ + if (skb->sk != sk || so->cfecho != *(u32 *)cf->data) + return;
- /* we did not get any flow control frame in time */ + /* cancel local echo timeout */ + hrtimer_cancel(&so->txtimer);
- /* report 'communication error on send' */ - sk->sk_err = ECOMM; - if (!sock_flag(sk, SOCK_DEAD)) - sk_error_report(sk); + /* local echo skb with consecutive frame has been consumed */ + so->cfecho = 0;
- /* reset tx state */ + if (so->tx.idx >= so->tx.len) { + /* we are done */ so->tx.state = ISOTP_IDLE; wake_up_interruptible(&so->wait); - break; - - case ISOTP_SENDING: - - /* push out the next segmented pdu */ - dev = dev_get_by_index(sock_net(sk), so->ifindex); - if (!dev) - break; + return; + }
-isotp_tx_burst: - skb = alloc_skb(so->ll.mtu + sizeof(struct can_skb_priv), - GFP_ATOMIC); - if (!skb) { - dev_put(dev); - break; - } + if (so->txfc.bs && so->tx.bs >= so->txfc.bs) { + /* stop and wait for FC with timeout */ + so->tx.state = ISOTP_WAIT_FC; + hrtimer_start(&so->txtimer, ktime_set(ISOTP_FC_TIMEOUT, 0), + HRTIMER_MODE_REL_SOFT); + return; + }
- can_skb_reserve(skb); - can_skb_prv(skb)->ifindex = dev->ifindex; - can_skb_prv(skb)->skbcnt = 0; + /* no gap between data frames needed => use burst mode */ + if (!so->tx_gap) { + /* enable echo timeout handling */ + hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0), + HRTIMER_MODE_REL_SOFT); + isotp_send_cframe(so); + return; + }
- cf = (struct canfd_frame *)skb->data; - skb_put_zero(skb, so->ll.mtu); + /* start timer to send next consecutive frame with correct delay */ + hrtimer_start(&so->txfrtimer, so->tx_gap, HRTIMER_MODE_REL_SOFT); +}
- /* create consecutive frame */ - isotp_fill_dataframe(cf, so, ae, 0); +static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer) +{ + struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, + txtimer); + struct sock *sk = &so->sk;
- /* place consecutive frame N_PCI in appropriate index */ - cf->data[ae] = N_PCI_CF | so->tx.sn++; - so->tx.sn %= 16; - so->tx.bs++; + /* don't handle timeouts in IDLE or SHUTDOWN state */ + if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN) + return HRTIMER_NORESTART;
- cf->flags = so->ll.tx_flags; + /* we did not get any flow control or echo frame in time */
- skb->dev = dev; - can_skb_set_owner(skb, sk); + /* report 'communication error on send' */ + sk->sk_err = ECOMM; + if (!sock_flag(sk, SOCK_DEAD)) + sk_error_report(sk);
- can_send_ret = can_send(skb, 1); - if (can_send_ret) { - pr_notice_once("can-isotp: %s: can_send_ret %pe\n", - __func__, ERR_PTR(can_send_ret)); - if (can_send_ret == -ENOBUFS) - pr_notice_once("can-isotp: tx queue is full, increasing txqueuelen may prevent this error\n"); - } - if (so->tx.idx >= so->tx.len) { - /* we are done */ - so->tx.state = ISOTP_IDLE; - dev_put(dev); - wake_up_interruptible(&so->wait); - break; - } + /* reset tx state */ + so->tx.state = ISOTP_IDLE; + wake_up_interruptible(&so->wait);
- if (so->txfc.bs && so->tx.bs >= so->txfc.bs) { - /* stop and wait for FC */ - so->tx.state = ISOTP_WAIT_FC; - dev_put(dev); - hrtimer_set_expires(&so->txtimer, - ktime_add(ktime_get(), - ktime_set(1, 0))); - restart = HRTIMER_RESTART; - break; - } + return HRTIMER_NORESTART; +}
- /* no gap between data frames needed => use burst mode */ - if (!so->tx_gap) - goto isotp_tx_burst; +static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer) +{ + struct isotp_sock *so = container_of(hrtimer, struct isotp_sock, + txfrtimer);
- /* start timer to send next data frame with correct delay */ - dev_put(dev); - hrtimer_set_expires(&so->txtimer, - ktime_add(ktime_get(), so->tx_gap)); - restart = HRTIMER_RESTART; - break; + /* start echo timeout handling and cover below protocol error */ + hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0), + HRTIMER_MODE_REL_SOFT);
- default: - WARN_ON_ONCE(1); - } + /* cfecho should be consumed by isotp_rcv_echo() here */ + if (so->tx.state == ISOTP_SENDING && !so->cfecho) + isotp_send_cframe(so);
- return restart; + return HRTIMER_NORESTART; }
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; int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0; int wait_tx_done = (so->opt.flags & CAN_ISOTP_WAIT_TX_DONE) ? 1 : 0; - s64 hrtimer_sec = 0; + s64 hrtimer_sec = ISOTP_ECHO_TIMEOUT; 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; + + 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) { err = -EINVAL; goto err_out_drop; @@ -894,11 +949,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
/* take care of a potential SF_DL ESC offset for TX_DL > 8 */ off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
/* does the given data fit into a single frame for SF_BROADCAST? */ - if ((so->opt.flags & CAN_ISOTP_SF_BROADCAST) && + if ((isotp_bc_flags(so) == CAN_ISOTP_SF_BROADCAST) && (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off)) { err = -EINVAL; goto err_out_drop; }
@@ -927,10 +982,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) so->tx.idx = 0;
cf = (struct canfd_frame *)skb->data; skb_put_zero(skb, so->ll.mtu);
+ /* cfecho should have been zero'ed by init / former isotp_rcv_echo() */ + if (so->cfecho) + pr_notice_once("can-isotp: uninit cfecho %08X\n", so->cfecho); + /* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl - SF_PCI_SZ4 - ae - off) { /* The message size generally fits into a SingleFrame - good. * * SF_DL ESC offset optimization: @@ -952,26 +1011,44 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) if (off) cf->data[SF_PCI_SZ4 + ae] = size; else cf->data[ae] |= size;
- so->tx.state = ISOTP_IDLE; - wake_up_interruptible(&so->wait); - - /* don't enable wait queue for a single frame transmission */ - wait_tx_done = 0; + /* set CF echo tag for isotp_rcv_echo() (SF-mode) */ + so->cfecho = *(u32 *)cf->data; } else { - /* send first frame and wait for FC */ + /* send first frame */
isotp_create_fframe(cf, so, ae);
- /* start timeout for FC */ - hrtimer_sec = 1; - hrtimer_start(&so->txtimer, ktime_set(hrtimer_sec, 0), - HRTIMER_MODE_REL_SOFT); + if (isotp_bc_flags(so) == CAN_ISOTP_CF_BROADCAST) { + /* set timer for FC-less operation (STmin = 0) */ + if (so->opt.flags & CAN_ISOTP_FORCE_TXSTMIN) + so->tx_gap = ktime_set(0, so->force_tx_stmin); + else + so->tx_gap = ktime_set(0, so->frame_txtime); + + /* disable wait for FCs due to activated block size */ + so->txfc.bs = 0; + + /* set CF echo tag for isotp_rcv_echo() (CF-mode) */ + so->cfecho = *(u32 *)cf->data; + } else { + /* standard flow control check */ + so->tx.state = ISOTP_WAIT_FIRST_FC; + + /* start timeout for FC */ + hrtimer_sec = ISOTP_FC_TIMEOUT; + + /* no CF echo tag for isotp_rcv_echo() (FF-mode) */ + so->cfecho = 0; + } }
+ hrtimer_start(&so->txtimer, ktime_set(hrtimer_sec, 0), + HRTIMER_MODE_REL_SOFT); + /* send the first or only CAN frame */ cf->flags = so->ll.tx_flags;
skb->dev = dev; skb->sk = sk; @@ -980,34 +1057,40 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) if (err) { pr_notice_once("can-isotp: %s: can_send_ret %pe\n", __func__, ERR_PTR(err));
/* no transmission -> no timeout monitoring */ - if (hrtimer_sec) - hrtimer_cancel(&so->txtimer); + hrtimer_cancel(&so->txtimer); + + /* reset consecutive frame echo tag */ + so->cfecho = 0;
goto err_out_drop; }
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;
err = sock_error(sk); if (err) return err; }
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; }
static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t size, @@ -1067,14 +1150,16 @@ static int isotp_release(struct socket *sock)
so = isotp_sk(sk); 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); while (isotp_busy_notifier == so) { spin_unlock(&isotp_notifier_lock); @@ -1085,25 +1170,31 @@ static int isotp_release(struct socket *sock) spin_unlock(&isotp_notifier_lock);
lock_sock(sk);
/* remove current filters & unregister */ - if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST))) { + if (so->bound) { if (so->ifindex) { struct net_device *dev;
dev = dev_get_by_index(net, so->ifindex); if (dev) { - can_rx_unregister(net, dev, so->rxid, - SINGLE_MASK(so->rxid), - isotp_rcv, sk); + if (isotp_register_rxid(so)) + can_rx_unregister(net, dev, so->rxid, + SINGLE_MASK(so->rxid), + isotp_rcv, sk); + + can_rx_unregister(net, dev, so->txid, + SINGLE_MASK(so->txid), + isotp_rcv_echo, sk); dev_put(dev); synchronize_rcu(); } } }
+ hrtimer_cancel(&so->txfrtimer); hrtimer_cancel(&so->txtimer); hrtimer_cancel(&so->rxtimer);
so->ifindex = 0; so->bound = 0; @@ -1126,11 +1217,10 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) int ifindex; struct net_device *dev; canid_t tx_id, rx_id; int err = 0; int notify_enetdown = 0; - int do_rx_reg = 1;
if (len < ISOTP_MIN_NAMELEN) return -EINVAL;
if (addr->can_family != AF_CAN) @@ -1162,16 +1252,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) if (so->bound) { err = -EINVAL; goto out; }
- /* do not register frame reception for functional addressing */ - if (so->opt.flags & CAN_ISOTP_SF_BROADCAST) - do_rx_reg = 0; - - /* do not validate rx address for functional addressing */ - if (do_rx_reg && rx_id == tx_id) { + /* ensure different CAN IDs when the rx_id is to be registered */ + if (isotp_register_rxid(so) && rx_id == tx_id) { err = -EADDRNOTAVAIL; goto out; }
dev = dev_get_by_index(net, addr->can_ifindex); @@ -1192,14 +1278,21 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) if (!(dev->flags & IFF_UP)) notify_enetdown = 1;
ifindex = dev->ifindex;
- if (do_rx_reg) + if (isotp_register_rxid(so)) can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id), isotp_rcv, sk, "isotp", sk);
+ /* no consecutive frame echo skb in flight */ + so->cfecho = 0; + + /* register for echo skb's */ + can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id), + isotp_rcv_echo, sk, "isotpe", sk); + dev_put(dev);
/* switch to new settings */ so->ifindex = ifindex; so->rxid = rx_id; @@ -1256,10 +1349,19 @@ static int isotp_setsockopt_locked(struct socket *sock, int level, int optname,
/* no separate rx_ext_address is given => use ext_address */ if (!(so->opt.flags & CAN_ISOTP_RX_EXT_ADDR)) so->opt.rx_ext_address = so->opt.ext_address;
+ /* these broadcast flags are not allowed together */ + if (isotp_bc_flags(so) == ISOTP_ALL_BC_FLAGS) { + /* CAN_ISOTP_SF_BROADCAST is prioritized */ + so->opt.flags &= ~CAN_ISOTP_CF_BROADCAST; + + /* give user feedback on wrong config attempt */ + ret = -EINVAL; + } + /* check for frame_txtime changes (0 => no changes) */ if (so->opt.frame_txtime) { if (so->opt.frame_txtime == CAN_ISOTP_FRAME_TXTIME_ZERO) so->frame_txtime = 0; else @@ -1406,14 +1508,20 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
switch (msg) { case NETDEV_UNREGISTER: lock_sock(sk); /* remove current filters & unregister */ - if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST))) - can_rx_unregister(dev_net(dev), dev, so->rxid, - SINGLE_MASK(so->rxid), - isotp_rcv, sk); + if (so->bound) { + if (isotp_register_rxid(so)) + can_rx_unregister(dev_net(dev), dev, so->rxid, + SINGLE_MASK(so->rxid), + isotp_rcv, sk); + + can_rx_unregister(dev_net(dev), dev, so->txid, + SINGLE_MASK(so->txid), + isotp_rcv_echo, sk); + }
so->ifindex = 0; so->bound = 0; release_sock(sk);
@@ -1482,10 +1590,12 @@ static int isotp_init(struct sock *sk)
hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT); so->rxtimer.function = isotp_rx_timer_handler; hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT); so->txtimer.function = isotp_tx_timer_handler; + hrtimer_init(&so->txfrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT); + so->txfrtimer.function = isotp_txfr_timer_handler;
init_waitqueue_head(&so->wait); spin_lock_init(&so->rx_lock);
spin_lock(&isotp_notifier_lock);
commit b76b163f46b661499921a0049982764a6659bfe7 upstream
With commit 2aa39889c463 ("can: isotp: isotp_bind(): return -EINVAL on incorrect CAN ID formatting") the bind() syscall returns -EINVAL when the given CAN ID needed to be sanitized. But in the case of an unconfirmed broadcast mode the rx CAN ID is not needed and may be uninitialized from the caller - which is ok.
This patch makes sure the result of an inproper CAN ID format is only provided when the address information is needed.
Link: https://lore.kernel.org/all/20220517145653.2556-1-socketcan@hartkopp.net Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de --- net/can/isotp.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c index 19fffa8676f9..00cb38b4a6f4 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1214,38 +1214,43 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) struct sock *sk = sock->sk; struct isotp_sock *so = isotp_sk(sk); struct net *net = sock_net(sk); int ifindex; struct net_device *dev; - canid_t tx_id, rx_id; + canid_t tx_id = addr->can_addr.tp.tx_id; + canid_t rx_id = addr->can_addr.tp.rx_id; int err = 0; int notify_enetdown = 0;
if (len < ISOTP_MIN_NAMELEN) return -EINVAL;
if (addr->can_family != AF_CAN) return -EINVAL;
- /* sanitize tx/rx CAN identifiers */ - tx_id = addr->can_addr.tp.tx_id; + /* sanitize tx CAN identifier */ if (tx_id & CAN_EFF_FLAG) tx_id &= (CAN_EFF_FLAG | CAN_EFF_MASK); else tx_id &= CAN_SFF_MASK;
- rx_id = addr->can_addr.tp.rx_id; - if (rx_id & CAN_EFF_FLAG) - rx_id &= (CAN_EFF_FLAG | CAN_EFF_MASK); - else - rx_id &= CAN_SFF_MASK; - - /* give feedback on wrong CAN-ID values */ - if (tx_id != addr->can_addr.tp.tx_id || - rx_id != addr->can_addr.tp.rx_id) + /* give feedback on wrong CAN-ID value */ + if (tx_id != addr->can_addr.tp.tx_id) return -EINVAL;
+ /* sanitize rx CAN identifier (if needed) */ + if (isotp_register_rxid(so)) { + if (rx_id & CAN_EFF_FLAG) + rx_id &= (CAN_EFF_FLAG | CAN_EFF_MASK); + else + rx_id &= CAN_SFF_MASK; + + /* give feedback on wrong CAN-ID value */ + if (rx_id != addr->can_addr.tp.rx_id) + return -EINVAL; + } + if (!addr->can_ifindex) return -ENODEV;
lock_sock(sk);
From: Lukas Magel lukas.magel@posteo.net
[ Upstream commit d9c2ba65e651467de739324d978b04ed8729f483 ]
With patch [1], isotp_poll was updated to also queue the poller in the so->wait queue, which is used for send state changes. Since the queue now also contains polling tasks that are not interested in sending, the queue fill state can no longer be used as an indication of send readiness. As a consequence, nonblocking writes can lead to a race and lock-up of the socket if there is a second task polling the socket in parallel.
With this patch, isotp_sendmsg does not consult wq_has_sleepers but instead tries to atomically set so->tx.state and waits on so->wait if it is unable to do so. This behavior is in alignment with isotp_poll, which also checks so->tx.state to determine send readiness.
V2: - Revert direct exit to goto err_event_drop
[1] https://lore.kernel.org/all/20230331125511.372783-1-michal.sojka@cvut.cz
Reported-by: Maxime Jayat maxime.jayat@mobile-devices.fr Closes: https://lore.kernel.org/linux-can/11328958-453f-447f-9af8-3b5824dfb041@munic... Signed-off-by: Lukas Magel lukas.magel@posteo.net Reviewed-by: Oliver Hartkopp socketcan@hartkopp.net Fixes: 79e19fa79cb5 ("can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events") Link: https://github.com/pylessard/python-udsoncan/issues/178#issuecomment-1743786... Link: https://lore.kernel.org/all/20230827092205.7908-1-lukas.magel@posteo.net Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de Signed-off-by: Sasha Levin sashal@kernel.org --- net/can/isotp.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c index 00cb38b4a6f4..7f62628c6ddd 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -923,25 +923,22 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) int err;
if (!so->bound || so->tx.state == ISOTP_SHUTDOWN) return -EADDRNOTAVAIL;
-wait_free_buffer: - /* we do not support multiple buffers - for now */ - if (wq_has_sleeper(&so->wait) && (msg->msg_flags & MSG_DONTWAIT)) - return -EAGAIN; + while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) { + /* we do not support multiple buffers - for now */ + if (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_event_drop; - - if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) { if (so->tx.state == ISOTP_SHUTDOWN) return -EADDRNOTAVAIL;
- goto wait_free_buffer; + /* wait for complete transmission of current pdu */ + err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); + if (err) + goto err_event_drop; }
if (!size || size > MAX_MSG_LENGTH) { err = -EINVAL; goto err_out_drop;
linux-stable-mirror@lists.linaro.org
-
Oliver Hartkopp