netdev - Re: [PATCH net v2] ice: clear number of qs when rings are free

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:   Mon, 20 Mar 2023 15:59:14 +0100
From:   Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        piotr.raczynski@...el.com,
        Mateusz Palczewski <mateusz.palczewski@...el.com>
Subject: Re: [PATCH net v2] ice: clear number of qs when rings are free

On Mon, Mar 20, 2023 at 01:51:17PM +0200, Leon Romanovsky wrote:
> On Mon, Mar 20, 2023 at 12:23:47PM +0100, Michal Swiatkowski wrote:
> > In case rebuild fails not clearing this field can lead to call trace.
> > 
> > [  +0.009792] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [  +0.000009] #PF: supervisor read access in kernel mode
> > [  +0.000006] #PF: error_code(0x0000) - not-present page
> > [  +0.000005] PGD 0 P4D 0
> > [  +0.000009] Oops: 0000 [#1] PREEMPT SMP PTI
> > [  +0.000009] CPU: 45 PID: 77867 Comm: ice-ptp-0000:60 Kdump: loaded Tainted: G S         OE      6.2.0-rc6+ #110
> > [  +0.000010] Hardware name: Dell Inc. PowerEdge R740/0JMK61, BIOS 2.11.2 004/21/2021
> > [  +0.000005] RIP: 0010:ice_ptp_update_cached_phctime+0xb0/0x130 [ice]
> > [  +0.000145] Code: fa 7e 55 48 8b 93 48 01 00 00 48 8b 0c fa 48 85 c9 74 e1 8b 51 68 85 d2 75 da 66 83 b9 86 04 00 00 00 74 d0 31 d2 48 8b 71 20 <48> 8b 34 d6 48 85 f6 74 07 48 89 86 d8 00 00 00 0f b7 b1 86 04 00
> > [  +0.000008] RSP: 0018:ffffa036cf7c7ea8 EFLAGS: 00010246
> > [  +0.000008] RAX: 174ab1a8ab400f43 RBX: ffff937cda2c01a0 RCX: ffff937cdca9b028
> > [  +0.000005] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > [  +0.000005] RBP: ffffa036cf7c7eb8 R08: 0000000000000000 R09: 0000000000000000
> > [  +0.000005] R10: 0000000000000080 R11: 0000000000000001 R12: ffff937cdc971f40
> > [  +0.000006] R13: ffff937cdc971f44 R14: 0000000000000001 R15: ffffffffc13f3210
> > [  +0.000005] FS:  0000000000000000(0000) GS:ffff93826f980000(0000) knlGS:0000000000000000
> > [  +0.000006] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [  +0.000006] CR2: 0000000000000000 CR3: 00000004b7310002 CR4: 00000000007726e0
> > [  +0.000006] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [  +0.000004] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [  +0.000005] PKRU: 55555554
> > [  +0.000004] Call Trace:
> > [  +0.000004]  <TASK>
> > [  +0.000007]  ice_ptp_periodic_work+0x2a/0x60 [ice]
> > [  +0.000126]  kthread_worker_fn+0xa6/0x250
> > [  +0.000014]  ? __pfx_kthread_worker_fn+0x10/0x10
> > [  +0.000010]  kthread+0xfc/0x130
> > [  +0.000009]  ? __pfx_kthread+0x10/0x10
> > [  +0.000010]  ret_from_fork+0x29/0x50
> > 
> > ice_ptp_update_cached_phctime() is calling ice_for_each_rxq macro, in
> > case of rebuild fail the rx_ring is NULL and there is NULL pointer
> > dereference.
> > 
> > Also for future safety it is better to clear the size values for tx and
> > rx ring when they are cleared.
> > 
> > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> > Reported-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> > ---
> > v1 --> v2:
> >  * change subject to net and add fixes tag
> > ---
> >  drivers/net/ethernet/intel/ice/ice_lib.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> 
> It will be so great if all these ice_for_each_*(*, i) macros will go.
> They do nothing except hide basic for-loop.

Good point, I spent too much time searching which field is used there.
Will try to propose sth.

Thanks for review

> 
> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@...dia.com>