| 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
| ||
|
Message-ID: <20250528091038.4264-1-pchelkin@ispras.ru>
Date: Wed, 28 May 2025 12:10:37 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Marc Kleine-Budde <mkl@...gutronix.de>,
Vincent Mailhol <mailhol.vincent@...adoo.fr>,
Axel Forsman <axfo@...ser.com>
Cc: Fedor Pchelkin <pchelkin@...ras.ru>,
Jimmy Assarsson <extja@...ser.com>,
linux-can@...r.kernel.org,
linux-kernel@...r.kernel.org,
lvc-project@...uxtesting.org,
stable@...r.kernel.org
Subject: [PATCH] can: kvaser_pciefd: refine error prone echo_skb_max handling logic
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@...r.kernel.org
Signed-off-by: Fedor Pchelkin <pchelkin@...ras.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.0
Powered by blists - more mailing lists