netdev - Re: [PATCH net v2] ionic: fix use after netif_napi

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 12 Jun 2024 09:58:13 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Brett Creeley <bcreeley@....com>, Taehee Yoo <ap420073@...il.com>,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, brett.creeley@....com, drivers@...sando.io,
 netdev@...r.kernel.org
Cc: jacob.e.keller@...el.com
Subject: Re: [PATCH net v2] ionic: fix use after netif_napi_del()

On 6/12/2024 8:36 AM, Brett Creeley wrote:
> On 6/11/2024 11:04 PM, Taehee Yoo wrote:
>> Caution: This message originated from an External Source. Use proper 
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> When queues are started, netif_napi_add() and napi_enable() are called.
>> If there are 4 queues and only 3 queues are used for the current
>> configuration, only 3 queues' napi should be registered and enabled.
>> The ionic_qcq_enable() checks whether the .poll pointer is not NULL for
>> enabling only the using queue' napi. Unused queues' napi will not be
>> registered by netif_napi_add(), so the .poll pointer indicates NULL.
>> But it couldn't distinguish whether the napi was unregistered or not
>> because netif_napi_del() doesn't reset the .poll pointer to NULL.
>> So, ionic_qcq_enable() calls napi_enable() for the queue, which was
>> unregistered by netif_napi_del().
>>
>> Reproducer:
>>     ethtool -L <interface name> rx 1 tx 1 combined 0
>>     ethtool -L <interface name> rx 0 tx 0 combined 1
>>     ethtool -L <interface name> rx 0 tx 0 combined 4
>>
>> Splat looks like:
>> kernel BUG at net/core/dev.c:6666!
>> Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
>> CPU: 3 PID: 1057 Comm: kworker/3:3 Not tainted 6.10.0-rc2+ #16
>> Workqueue: events ionic_lif_deferred_work [ionic]
>> RIP: 0010:napi_enable+0x3b/0x40
>> Code: 48 89 c2 48 83 e2 f6 80 b9 61 09 00 00 00 74 0d 48 83 bf 60 01 
>> 00 00 00 74 03 80 ce 01 f0 4f
>> RSP: 0018:ffffb6ed83227d48 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: ffff97560cda0828 RCX: 0000000000000029
>> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff97560cda0a28
>> RBP: ffffb6ed83227d50 R08: 0000000000000400 R09: 0000000000000001
>> R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
>> R13: ffff97560ce3c1a0 R14: 0000000000000000 R15: ffff975613ba0a20
>> FS:  0000000000000000(0000) GS:ffff975d5f780000(0000) 
>> knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007f8f734ee200 CR3: 0000000103e50000 CR4: 00000000007506f0
>> PKRU: 55555554
>> Call Trace:
>>   <TASK>
>>   ? die+0x33/0x90
>>   ? do_trap+0xd9/0x100
>>   ? napi_enable+0x3b/0x40
>>   ? do_error_trap+0x83/0xb0
>>   ? napi_enable+0x3b/0x40
>>   ? napi_enable+0x3b/0x40
>>   ? exc_invalid_op+0x4e/0x70
>>   ? napi_enable+0x3b/0x40
>>   ? asm_exc_invalid_op+0x16/0x20
>>   ? napi_enable+0x3b/0x40
>>   ionic_qcq_enable+0xb7/0x180 [ionic 
>> 59bdfc8a035436e1c4224ff7d10789e3f14643f8]
>>   ionic_start_queues+0xc4/0x290 [ionic 
>> 59bdfc8a035436e1c4224ff7d10789e3f14643f8]
>>   ionic_link_status_check+0x11c/0x170 [ionic 
>> 59bdfc8a035436e1c4224ff7d10789e3f14643f8]
>>   ionic_lif_deferred_work+0x129/0x280 [ionic 
>> 59bdfc8a035436e1c4224ff7d10789e3f14643f8]
>>   process_one_work+0x145/0x360
>>   worker_thread+0x2bb/0x3d0
>>   ? __pfx_worker_thread+0x10/0x10
>>   kthread+0xcc/0x100
>>   ? __pfx_kthread+0x10/0x10
>>   ret_from_fork+0x2d/0x50
>>   ? __pfx_kthread+0x10/0x10
>>   ret_from_fork_asm+0x1a/0x30
>>
>> Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling")
>> Signed-off-by: Taehee Yoo <ap420073@...il.com>
>> ---
>>   v2:
>>    - Use ionic flag instead of napi flag.
>>
>>   drivers/net/ethernet/pensando/ionic/ionic_lif.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c 
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index 24870da3f484..1934e9d6d9e4 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -304,10 +304,8 @@ static int ionic_qcq_enable(struct ionic_qcq *qcq)
>>          if (ret)
>>                  return ret;
>>
>> -       if (qcq->napi.poll)
>> -               napi_enable(&qcq->napi);
>> -
>>          if (qcq->flags & IONIC_QCQ_F_INTR) {
>> +               napi_enable(&qcq->napi);
> 
> LGTM. Thanks for finding/fixing this!
> 
> Reviewed-by: Brett Creeley <brett.creeley@....com>

Yes, thanks.

Reviewed-by: Shannon Nelson <shannon.nelson@....com>

> 
>>                  irq_set_affinity_hint(qcq->intr.vector,
>>                                        &qcq->intr.affinity_mask);
>>                  ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
>> -- 
>> 2.34.1
>>