From: "Chia-Lin Kao (AceLan)" <[email protected]>
The final update statement of the for loop exceeds the array range, the
dereference of self->aq_vec[i] is not checked and then leads to the
index out of range error.
Also fixed this kind of coding style in other for loop.
[ 97.937604] UBSAN: array-index-out-of-bounds in drivers/net/ethernet/aquantia/atlantic/aq_nic.c:1404:48
[ 97.937607] index 8 is out of range for type 'aq_vec_s *[8]'
[ 97.937608] CPU: 38 PID: 3767 Comm: kworker/u256:18 Not tainted 5.19.0+ #2
[ 97.937610] Hardware name: Dell Inc. Precision 7865 Tower/, BIOS 1.0.0 06/12/2022
[ 97.937611] Workqueue: events_unbound async_run_entry_fn
[ 97.937616] Call Trace:
[ 97.937617] <TASK>
[ 97.937619] dump_stack_lvl+0x49/0x63
[ 97.937624] dump_stack+0x10/0x16
[ 97.937626] ubsan_epilogue+0x9/0x3f
[ 97.937627] __ubsan_handle_out_of_bounds.cold+0x44/0x49
[ 97.937629] ? __scm_send+0x348/0x440
[ 97.937632] ? aq_vec_stop+0x72/0x80 [atlantic]
[ 97.937639] aq_nic_stop+0x1b6/0x1c0 [atlantic]
[ 97.937644] aq_suspend_common+0x88/0x90 [atlantic]
[ 97.937648] aq_pm_suspend_poweroff+0xe/0x20 [atlantic]
[ 97.937653] pci_pm_suspend+0x7e/0x1a0
[ 97.937655] ? pci_pm_suspend_noirq+0x2b0/0x2b0
[ 97.937657] dpm_run_callback+0x54/0x190
[ 97.937660] __device_suspend+0x14c/0x4d0
[ 97.937661] async_suspend+0x23/0x70
[ 97.937663] async_run_entry_fn+0x33/0x120
[ 97.937664] process_one_work+0x21f/0x3f0
[ 97.937666] worker_thread+0x4a/0x3c0
[ 97.937668] ? process_one_work+0x3f0/0x3f0
[ 97.937669] kthread+0xf0/0x120
[ 97.937671] ? kthread_complete_and_exit+0x20/0x20
[ 97.937672] ret_from_fork+0x22/0x30
[ 97.937676] </TASK>
v2. fixed "warning: variable 'aq_vec' set but not used"
Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
Signed-off-by: Chia-Lin Kao (AceLan) <[email protected]>
---
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index e11cc29d3264..6986f0080072 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -265,12 +265,10 @@ static void aq_nic_service_timer_cb(struct timer_list *t)
static void aq_nic_polling_timer_cb(struct timer_list *t)
{
struct aq_nic_s *self = from_timer(self, t, polling_timer);
- struct aq_vec_s *aq_vec = NULL;
unsigned int i = 0U;
- for (i = 0U, aq_vec = self->aq_vec[0];
- self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i])
- aq_vec_isr(i, (void *)aq_vec);
+ for (i = 0U; self->aq_vecs > i; ++i)
+ aq_vec_isr(i, (void *)self->aq_vec[i]);
mod_timer(&self->polling_timer, jiffies +
AQ_CFG_POLLING_TIMER_INTERVAL);
@@ -1065,10 +1063,11 @@ u64 *aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
for (tc = 0U; tc < self->aq_nic_cfg.tcs; tc++) {
for (i = 0U, aq_vec = self->aq_vec[0];
- aq_vec && self->aq_vecs > i;
- ++i, aq_vec = self->aq_vec[i]) {
+ aq_vec && self->aq_vecs > i; ++i) {
data += count;
count = aq_vec_get_sw_stats(aq_vec, tc, data);
+ if (self->aq_vecs > i)
+ aq_vec = self->aq_vec[i];
}
}
@@ -1382,7 +1381,6 @@ int aq_nic_set_loopback(struct aq_nic_s *self)
int aq_nic_stop(struct aq_nic_s *self)
{
- struct aq_vec_s *aq_vec = NULL;
unsigned int i = 0U;
netif_tx_disable(self->ndev);
@@ -1400,9 +1398,8 @@ int aq_nic_stop(struct aq_nic_s *self)
aq_ptp_irq_free(self);
- for (i = 0U, aq_vec = self->aq_vec[0];
- self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i])
- aq_vec_stop(aq_vec);
+ for (i = 0U; self->aq_vecs > i; ++i)
+ aq_vec_stop(self->aq_vec[i]);
aq_ptp_ring_stop(self);
--
2.25.1
> -----Original Message-----
> From: AceLan Kao <[email protected]> On Behalf Of AceLan Kao
> Sent: Saturday, August 6, 2022 7:06 PM
> To: Igor Russkikh <[email protected]>; David S. Miller
> <[email protected]>; Eric Dumazet <[email protected]>; Jakub
> Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Dmitrii
> Tarakanov <[email protected]>; Alexander Loktionov
> <[email protected]>; David VomLehn
> <[email protected]>; Dmitry Bezrukov <[email protected]>;
> [email protected]; [email protected]
> Subject: [PATCH v2] net: atlantic: fix aq_vec index out of range error
>
> From: "Chia-Lin Kao (AceLan)" <[email protected]>
>
> The final update statement of the for loop exceeds the array range, the
> dereference of self->aq_vec[i] is not checked and then leads to the index out
> of range error.
> Also fixed this kind of coding style in other for loop.
>
> [ 97.937604] UBSAN: array-index-out-of-bounds in
> drivers/net/ethernet/aquantia/atlantic/aq_nic.c:1404:48
> [ 97.937607] index 8 is out of range for type 'aq_vec_s *[8]'
> [ 97.937608] CPU: 38 PID: 3767 Comm: kworker/u256:18 Not tainted 5.19.0+
> #2
> [ 97.937610] Hardware name: Dell Inc. Precision 7865 Tower/, BIOS 1.0.0
> 06/12/2022
> [ 97.937611] Workqueue: events_unbound async_run_entry_fn
> [ 97.937616] Call Trace:
> [ 97.937617] <TASK>
> [ 97.937619] dump_stack_lvl+0x49/0x63
> [ 97.937624] dump_stack+0x10/0x16
> [ 97.937626] ubsan_epilogue+0x9/0x3f
> [ 97.937627] __ubsan_handle_out_of_bounds.cold+0x44/0x49
> [ 97.937629] ? __scm_send+0x348/0x440
> [ 97.937632] ? aq_vec_stop+0x72/0x80 [atlantic]
> [ 97.937639] aq_nic_stop+0x1b6/0x1c0 [atlantic]
> [ 97.937644] aq_suspend_common+0x88/0x90 [atlantic]
> [ 97.937648] aq_pm_suspend_poweroff+0xe/0x20 [atlantic]
> [ 97.937653] pci_pm_suspend+0x7e/0x1a0
> [ 97.937655] ? pci_pm_suspend_noirq+0x2b0/0x2b0
> [ 97.937657] dpm_run_callback+0x54/0x190
> [ 97.937660] __device_suspend+0x14c/0x4d0
> [ 97.937661] async_suspend+0x23/0x70
> [ 97.937663] async_run_entry_fn+0x33/0x120
> [ 97.937664] process_one_work+0x21f/0x3f0
> [ 97.937666] worker_thread+0x4a/0x3c0
> [ 97.937668] ? process_one_work+0x3f0/0x3f0
> [ 97.937669] kthread+0xf0/0x120
> [ 97.937671] ? kthread_complete_and_exit+0x20/0x20
> [ 97.937672] ret_from_fork+0x22/0x30
> [ 97.937676] </TASK>
>
> v2. fixed "warning: variable 'aq_vec' set but not used"
>
> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific
> code")
> Signed-off-by: Chia-Lin Kao (AceLan) <[email protected]>
> ---
> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index e11cc29d3264..6986f0080072 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -265,12 +265,10 @@ static void aq_nic_service_timer_cb(struct
> timer_list *t) static void aq_nic_polling_timer_cb(struct timer_list *t) {
> struct aq_nic_s *self = from_timer(self, t, polling_timer);
> - struct aq_vec_s *aq_vec = NULL;
> unsigned int i = 0U;
>
> - for (i = 0U, aq_vec = self->aq_vec[0];
> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i])
> - aq_vec_isr(i, (void *)aq_vec);
> + for (i = 0U; self->aq_vecs > i; ++i)
> + aq_vec_isr(i, (void *)self->aq_vec[i]);
>
> mod_timer(&self->polling_timer, jiffies +
> AQ_CFG_POLLING_TIMER_INTERVAL);
> @@ -1065,10 +1063,11 @@ u64 *aq_nic_get_stats(struct aq_nic_s *self, u64
> *data)
>
> for (tc = 0U; tc < self->aq_nic_cfg.tcs; tc++) {
> for (i = 0U, aq_vec = self->aq_vec[0];
> - aq_vec && self->aq_vecs > i;
> - ++i, aq_vec = self->aq_vec[i]) {
> + aq_vec && self->aq_vecs > i; ++i) {
> data += count;
> count = aq_vec_get_sw_stats(aq_vec, tc, data);
> + if (self->aq_vecs > i)
> + aq_vec = self->aq_vec[i];
> }
AeLan, thanks for the changes. Could you please simplify this logic as well (like other instances). Say,
for (i = 0U; self->aq_vecs > i; ++i) {
...
count = aq_vec_get_sw_stats(self->aq_vec[i], tc, data);
> }
>
> @@ -1382,7 +1381,6 @@ int aq_nic_set_loopback(struct aq_nic_s *self)
>
> int aq_nic_stop(struct aq_nic_s *self)
> {
> - struct aq_vec_s *aq_vec = NULL;
> unsigned int i = 0U;
>
> netif_tx_disable(self->ndev);
> @@ -1400,9 +1398,8 @@ int aq_nic_stop(struct aq_nic_s *self)
>
> aq_ptp_irq_free(self);
>
> - for (i = 0U, aq_vec = self->aq_vec[0];
> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i])
> - aq_vec_stop(aq_vec);
> + for (i = 0U; self->aq_vecs > i; ++i)
> + aq_vec_stop(self->aq_vec[i]);
>
> aq_ptp_ring_stop(self);
>
> --
> 2.25.1