drivers/net/can/kvaser_pciefd.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
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)
--
2.49.0Thanks 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.
© 2016 - 2025 Red Hat, Inc.