echo_skb_max should define the supported upper limit of echo_skb[] allocated inside the netdevice's priv. The corresponding size value provided by this driver to alloc_candev() is KVASER_PCIEFD_CAN_TX_MAX_COUNT which is 17.
But later echo_skb_max is rounded up to the nearest power of two (for the max case, that would be 32) and the tx/ack indices calculated further during tx/rx may exceed the upper array boundary. Kasan reported this for the ack case inside kvaser_pciefd_handle_ack_packet(), though the xmit function has actually caught the same thing earlier.
BUG: KASAN: slab-out-of-bounds in kvaser_pciefd_handle_ack_packet+0x2d7/0x92a drivers/net/can/kvaser_pciefd.c:1528 Read of size 8 at addr ffff888105e4f078 by task swapper/4/0
CPU: 4 UID: 0 PID: 0 Comm: swapper/4 Not tainted 6.15.0 #12 PREEMPT(voluntary) Call Trace: <IRQ> dump_stack_lvl lib/dump_stack.c:122 print_report mm/kasan/report.c:521 kasan_report mm/kasan/report.c:634 kvaser_pciefd_handle_ack_packet drivers/net/can/kvaser_pciefd.c:1528 kvaser_pciefd_read_packet drivers/net/can/kvaser_pciefd.c:1605 kvaser_pciefd_read_buffer drivers/net/can/kvaser_pciefd.c:1656 kvaser_pciefd_receive_irq drivers/net/can/kvaser_pciefd.c:1684 kvaser_pciefd_irq_handler drivers/net/can/kvaser_pciefd.c:1733 __handle_irq_event_percpu kernel/irq/handle.c:158 handle_irq_event kernel/irq/handle.c:210 handle_edge_irq kernel/irq/chip.c:833 __common_interrupt arch/x86/kernel/irq.c:296 common_interrupt arch/x86/kernel/irq.c:286 </IRQ>
Remove echo_skb_max rounding as this may increase it to unexpected values. In this sense restore echo_skb_max handling logic prior to commit 8256e0ca6010 ("can: kvaser_pciefd: Fix echo_skb race").
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 8256e0ca6010 ("can: kvaser_pciefd: Fix echo_skb race") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru ---
Actually the trick with rounding up allows to calculate seq numbers efficiently, avoiding a more consuming 'mod' operation used in the current patch.
I see tx max size definitely matters only for kvaser_pciefd_tx_avail(), but for seq numbers' generation that's not the case - we're free to calculate them as would be more convenient, not taking tx max size into account. The only downside is that the size of echo_skb[] should correspond to the max seq number (not tx max number), so in some situations a bit more memory would be consumed than could be.
So another approach to fix the problem would be to precompute the rounded up value of echo_skb_max and pass it to alloc_candev() making the size of the underlying echo_skb[] sufficient.
If that looks more acceptable, I'll be glad to rework the patch in that direction.
drivers/net/can/kvaser_pciefd.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c index f6921368cd14..1ec4ab9510b6 100644 --- a/drivers/net/can/kvaser_pciefd.c +++ b/drivers/net/can/kvaser_pciefd.c @@ -411,7 +411,6 @@ struct kvaser_pciefd_can { void __iomem *reg_base; struct can_berr_counter bec; u8 cmd_seq; - u8 tx_max_count; u8 tx_idx; u8 ack_idx; int err_rep_cnt; @@ -760,7 +759,7 @@ static int kvaser_pciefd_stop(struct net_device *netdev)
static unsigned int kvaser_pciefd_tx_avail(const struct kvaser_pciefd_can *can) { - return can->tx_max_count - (READ_ONCE(can->tx_idx) - READ_ONCE(can->ack_idx)); + return can->can.echo_skb_max - (READ_ONCE(can->tx_idx) - READ_ONCE(can->ack_idx)); }
static int kvaser_pciefd_prepare_tx_packet(struct kvaser_pciefd_tx_packet *p, @@ -810,7 +809,7 @@ static netdev_tx_t kvaser_pciefd_start_xmit(struct sk_buff *skb, { struct kvaser_pciefd_can *can = netdev_priv(netdev); struct kvaser_pciefd_tx_packet packet; - unsigned int seq = can->tx_idx & (can->can.echo_skb_max - 1); + unsigned int seq = can->tx_idx % can->can.echo_skb_max; unsigned int frame_len; int nr_words;
@@ -992,10 +991,9 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) tx_nr_packets_max = FIELD_GET(KVASER_PCIEFD_KCAN_TX_NR_PACKETS_MAX_MASK, ioread32(can->reg_base + KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG)); - can->tx_max_count = min(KVASER_PCIEFD_CAN_TX_MAX_COUNT, tx_nr_packets_max - 1); + can->can.echo_skb_max = min(KVASER_PCIEFD_CAN_TX_MAX_COUNT, tx_nr_packets_max - 1);
can->can.clock.freq = pcie->freq; - can->can.echo_skb_max = roundup_pow_of_two(can->tx_max_count); spin_lock_init(&can->lock);
can->can.bittiming_const = &kvaser_pciefd_bittiming_const; @@ -1523,7 +1521,7 @@ static int kvaser_pciefd_handle_ack_packet(struct kvaser_pciefd *pcie, unsigned int len, frame_len = 0; struct sk_buff *skb;
- if (echo_idx != (can->ack_idx & (can->can.echo_skb_max - 1))) + if (echo_idx != can->ack_idx % can->can.echo_skb_max) return 0; skb = can->can.echo_skb[echo_idx]; if (!skb)
Thanks for finding and fixing this bug.
Fedor Pchelkin pchelkin@ispras.ru writes:
Actually the trick with rounding up allows to calculate seq numbers efficiently, avoiding a more consuming 'mod' operation used in the current patch.
Indeed, that was the intention.
So another approach to fix the problem would be to precompute the rounded up value of echo_skb_max and pass it to alloc_candev() making the size of the underlying echo_skb[] sufficient.
I believe that is preferable---if memory usage is a concern KVASER_PCIEFD_CAN_TX_MAX_COUNT could be lowered by one. Something like the following:
diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c index f6921368cd14..0071a51ce2c1 100644 --- a/drivers/net/can/kvaser_pciefd.c +++ b/drivers/net/can/kvaser_pciefd.c @@ -966,7 +966,7 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) u32 status, tx_nr_packets_max;
netdev = alloc_candev(sizeof(struct kvaser_pciefd_can), - KVASER_PCIEFD_CAN_TX_MAX_COUNT); + roundup_pow_of_two(KVASER_PCIEFD_CAN_TX_MAX_COUNT)); if (!netdev) return -ENOMEM;
@@ -995,7 +995,6 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) can->tx_max_count = min(KVASER_PCIEFD_CAN_TX_MAX_COUNT, tx_nr_packets_max - 1);
can->can.clock.freq = pcie->freq; - can->can.echo_skb_max = roundup_pow_of_two(can->tx_max_count); spin_lock_init(&can->lock);
can->can.bittiming_const = &kvaser_pciefd_bittiming_const;
/Axel Forsman
On Wed, 28. May 13:32, Axel Forsman wrote:
Thanks for finding and fixing this bug.
Fedor Pchelkin pchelkin@ispras.ru writes:
Actually the trick with rounding up allows to calculate seq numbers efficiently, avoiding a more consuming 'mod' operation used in the current patch.
Indeed, that was the intention.
So another approach to fix the problem would be to precompute the rounded up value of echo_skb_max and pass it to alloc_candev() making the size of the underlying echo_skb[] sufficient.
I believe that is preferable---if memory usage is a concern KVASER_PCIEFD_CAN_TX_MAX_COUNT could be lowered by one. Something like the following:
diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c index f6921368cd14..0071a51ce2c1 100644 --- a/drivers/net/can/kvaser_pciefd.c +++ b/drivers/net/can/kvaser_pciefd.c @@ -966,7 +966,7 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) u32 status, tx_nr_packets_max;
netdev = alloc_candev(sizeof(struct kvaser_pciefd_can),
KVASER_PCIEFD_CAN_TX_MAX_COUNT);
roundup_pow_of_two(KVASER_PCIEFD_CAN_TX_MAX_COUNT)); if (!netdev) return -ENOMEM;@@ -995,7 +995,6 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie) can->tx_max_count = min(KVASER_PCIEFD_CAN_TX_MAX_COUNT, tx_nr_packets_max - 1);
can->can.clock.freq = pcie->freq;
can->can.echo_skb_max = roundup_pow_of_two(can->tx_max_count); spin_lock_init(&can->lock); can->can.bittiming_const = &kvaser_pciefd_bittiming_const;
Got it, thanks for review!
Setting KVASER_PCIEFD_CAN_TX_MAX_COUNT - value representing something like the count of pending tx frames - to 17 (not even a multiple of 2) is quite strange to me. This was probably done due to some hardware or protocol specs though I've failed to find any evidence available in public access.
Will send v2 soon.
linux-stable-mirror@lists.linaro.org
-
Axel Forsman -
Fedor Pchelkin