Add some missing `// SAFETY` comments #475

LeSeulArtichaut · 2021-08-01T00:06:06Z

Helps with #351.

Signed-off-by: Léo Lanteri Thauvin [email protected]

rust/kernel/miscdev.rs

LeSeulArtichaut · 2021-11-02T14:38:02Z

@wedsonaf will you have time to take a look at this? Should I request someone else's review?

rust/kernel/print.rs

rust/kernel/sync/condvar.rs

rust/kernel/pages.rs

rust/kernel/miscdev.rs

rust/kernel/file_operations.rs

ojeda · 2021-11-02T16:07:49Z

rust/kernel/print.rs

@@ -53,6 +53,7 @@ unsafe fn rust_fmt_argument(buf: *mut c_char, end: *mut c_char, ptr: *const c_vo
        buf: buf as _,
        end: end as _,
    };
+    // SAFETY: the caller must guarantee that `ptr` is valid.


Since this is an unsafe function, we can add this prerequisite to the function docs (in a Safety section), even if the function is not public; and then you can say here "ptr is valid due to the safety contract". You should also mention that ptr has to be an Arguments.

Documented the safety contract, though I'm not entirely sure it is correct

rust/kernel/print.rs

rust/kernel/pages.rs

ojeda · 2021-11-05T16:40:54Z

rust/kernel/pages.rs

+        // reading from, with no overflow. Also, `offset <= PAGE_SIZE < isize::MAX`
+        // so `offset` can't overflow an `isize`.
+        let src = unsafe { (mapping.ptr as *mut u8).add(offset) };
+        // SAFETY: `src` is valid for reads from the type invariants, and `dest` is


Hmm... it is valid for reads because we check len <= PAGE_SIZE, no?

I thought this was redundant with the previous comment

SAFETY: we just checked that offset + len <= PAGE_SIZE, so mapping.ptr and mapping.ptr + offset are in the same allocated object, the page we're reading from

I can repeat it if you prefer however.

rust/kernel/print.rs

when the number of DRR classes decreases, the round-robin active list can contain elements that have already been freed in ets_qdisc_change(). As a consequence, it's possible to see a NULL dereference crash, caused by the attempt to call cl->qdisc->ops->peek(cl->qdisc) when cl->qdisc is NULL: BUG: kernel NULL pointer dereference, address: 0000000000000018 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP NOPTI CPU: 1 PID: 910 Comm: mausezahn Not tainted 5.16.0-rc1+ #475 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014 RIP: 0010:ets_qdisc_dequeue+0x129/0x2c0 [sch_ets] Code: c5 01 41 39 ad e4 02 00 00 0f 87 18 ff ff ff 49 8b 85 c0 02 00 00 49 39 c4 0f 84 ba 00 00 00 49 8b ad c0 02 00 00 48 8b 7d 10 <48> 8b 47 18 48 8b 40 38 0f ae e8 ff d0 48 89 c3 48 85 c0 0f 84 9d RSP: 0000:ffffbb36c0b5fdd8 EFLAGS: 00010287 RAX: ffff956678efed30 RBX: 0000000000000000 RCX: 0000000000000000 RDX: 0000000000000002 RSI: ffffffff9b938dc9 RDI: 0000000000000000 RBP: ffff956678efed30 R08: e2f3207fe360129c R09: 0000000000000000 R10: 0000000000000001 R11: 0000000000000001 R12: ffff956678efeac0 R13: ffff956678efe800 R14: ffff956611545000 R15: ffff95667ac8f100 FS: 00007f2aa9120740(0000) GS:ffff95667b800000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000018 CR3: 000000011070c000 CR4: 0000000000350ee0 Call Trace: <TASK> qdisc_peek_dequeued+0x29/0x70 [sch_ets] tbf_dequeue+0x22/0x260 [sch_tbf] __qdisc_run+0x7f/0x630 net_tx_action+0x290/0x4c0 __do_softirq+0xee/0x4f8 irq_exit_rcu+0xf4/0x130 sysvec_apic_timer_interrupt+0x52/0xc0 asm_sysvec_apic_timer_interrupt+0x12/0x20 RIP: 0033:0x7f2aa7fc9ad4 Code: b9 ff ff 48 8b 54 24 18 48 83 c4 08 48 89 ee 48 89 df 5b 5d e9 ed fc ff ff 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 f3 0f 1e fa <53> 48 83 ec 10 48 8b 05 10 64 33 00 48 8b 00 48 85 c0 0f 85 84 00 RSP: 002b:00007ffe5d33fab8 EFLAGS: 00000202 RAX: 0000000000000002 RBX: 0000561f72c31460 RCX: 0000561f72c31720 RDX: 0000000000000002 RSI: 0000561f72c31722 RDI: 0000561f72c31720 RBP: 000000000000002a R08: 00007ffe5d33fa40 R09: 0000000000000014 R10: 0000000000000000 R11: 0000000000000246 R12: 0000561f7187e380 R13: 0000000000000000 R14: 0000000000000000 R15: 0000561f72c31460 </TASK> Modules linked in: sch_ets sch_tbf dummy rfkill iTCO_wdt intel_rapl_msr iTCO_vendor_support intel_rapl_common joydev virtio_balloon lpc_ich i2c_i801 i2c_smbus pcspkr ip_tables xfs libcrc32c crct10dif_pclmul crc32_pclmul crc32c_intel ahci libahci ghash_clmulni_intel serio_raw libata virtio_blk virtio_console virtio_net net_failover failover sunrpc dm_mirror dm_region_hash dm_log dm_mod CR2: 0000000000000018 Ensuring that 'alist' was never zeroed [1] was not sufficient, we need to remove from the active list those elements that are no more SP nor DRR. [1] https://lore.kernel.org/netdev/60d274838bf09777f0371253416e8af71360bc08.1633609148.git.dcaratti@redhat.com/ v3: fix race between ets_qdisc_change() and ets_qdisc_dequeue() delisting DRR classes beyond 'nbands' in ets_qdisc_change() with the qdisc lock acquired, thanks to Cong Wang. v2: when a NULL qdisc is found in the DRR active list, try to dequeue skb from the next list item. Reported-by: Hangbin Liu <[email protected]> Fixes: dcc68b4 ("net: sch_ets: Add a new Qdisc") Signed-off-by: Davide Caratti <[email protected]> Link: https://lore.kernel.org/r/7a5c496eed2d62241620bdbb83eb03fb9d571c99.1637762721.git.dcaratti@redhat.com Signed-off-by: Jakub Kicinski <[email protected]>

Signed-off-by: Léo Lanteri Thauvin <[email protected]>

Bevisy · 2022-09-28T08:28:57Z

Ping @LeSeulArtichaut merge conflicts

Will cause below warning then reboot when exercising the decoder with fluster on mt8192-asurada-spherion. This deinit function is called on the v4l2 release callback, even though the work might not have been initialized as that only happens if/when the codec specific 'decode' callback is called (as a result of device_run m2m callback). CPU: 5 PID: 2338 Comm: gst-launch-1.0 Tainted: G W 6.4.0-rc5-next-20230607+ #475 Hardware name: Google Spherion (rev0 - 3) (DT) pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : __flush_work.isra.0+0x23c/0x258 lr : __cancel_work_timer+0x14c/0x1c8 sp : ffff8000896e3b00 x29: ffff8000896e3b00 x28: ffff57c3d4079f80 x27: 0000000000000000 x26: ffff57c3d4079f80 x25: ffffb76395b59dc8 x24: 0000000000000001 x23: ffffb763928daab8 x22: ffff57c3d4079f80 x21: 0000000000000000 x20: ffffb763955f6778 x19: ffff57c3cf06f4a0 x18: 0000000000000000 x17: 000000040044ffff x16: 005000f2b5503510 x15: 0000000000000000 x14: ffff57c3c03a1f80 x13: ffffa0616a2fc000 x12: 000000003464d91d x11: 0000000000000000 x10: 0000000000001b10 x9 : ffffb763928de61c x8 : ffff57c3d407baf0 x7 : 0000000000000000 x6 : ffff57c3d4079f80 x5 : ffff57c3d4079f80 x4 : 0000000000000000 x3 : 0000000000000000 x2 : ffff8000896e3bf0 x1 : 0000000000000011 x0 : 0000000000000000 Call trace: __flush_work.isra.0+0x23c/0x258 __cancel_work_timer+0x14c/0x1c8 cancel_work_sync+0x1c/0x30 vdec_msg_queue_deinit+0xac/0xc8 vdec_h264_slice_deinit+0x64/0xb8 vdec_if_deinit+0x3c/0x68 mtk_vcodec_dec_release+0x20/0x40 fops_vcodec_release+0x50/0xd8 v4l2_release+0x7c/0x100 __fput+0x80/0x270 ____fput+0x18/0x30 task_work_run+0x78/0xe0 do_notify_resume+0x29c/0x7f8 el0_svc+0xa4/0xb8 el0t_64_sync_handler+0xc0/0xc8 el0t_64_sync+0x1a8/0x1b0 ---[ end trace 0000000000000000 ]--- Fixes: 297160d ("media: mediatek: vcodec: move core context from device to each instance") Reported-by: Nícolas F. R. A. Prado <[email protected]> Signed-off-by: Yunfei Dong <[email protected]> Reviewed-by: AngeloGioacchino Del Regno <[email protected]> Signed-off-by: Hans Verkuil <[email protected]> Signed-off-by: Mauro Carvalho Chehab <[email protected]>

Add a small test to write a (verification-time) fixed vs unknown but bounded-sized buffer into .rodata BPF map and assert that both get rejected. # ./vmtest.sh -- ./test_progs -t verifier_const [...] ./test_progs -t verifier_const [ 1.418717] tsc: Refined TSC clocksource calibration: 3407.994 MHz [ 1.419113] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fcde90a1, max_idle_ns: 440795222066 ns [ 1.419972] clocksource: Switched to clocksource tsc [ 1.449596] bpf_testmod: loading out-of-tree module taints kernel. [ 1.449958] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel Rust-for-Linux#475/1 verifier_const/rodata/strtol: write rejected:OK Rust-for-Linux#475/2 verifier_const/bss/strtol: write accepted:OK Rust-for-Linux#475/3 verifier_const/data/strtol: write accepted:OK Rust-for-Linux#475/4 verifier_const/rodata/mtu: write rejected:OK Rust-for-Linux#475/5 verifier_const/bss/mtu: write accepted:OK Rust-for-Linux#475/6 verifier_const/data/mtu: write accepted:OK Rust-for-Linux#475/7 verifier_const/rodata/mark: write with unknown reg rejected:OK Rust-for-Linux#475/8 verifier_const/rodata/mark: write with unknown reg rejected:OK Rust-for-Linux#475 verifier_const:OK Rust-for-Linux#476/1 verifier_const_or/constant register |= constant should keep constant type:OK Rust-for-Linux#476/2 verifier_const_or/constant register |= constant should not bypass stack boundary checks:OK Rust-for-Linux#476/3 verifier_const_or/constant register |= constant register should keep constant type:OK Rust-for-Linux#476/4 verifier_const_or/constant register |= constant register should not bypass stack boundary checks:OK Rust-for-Linux#476 verifier_const_or:OK Summary: 2/12 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <[email protected]> Acked-by: Kumar Kartikeya Dwivedi <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Alexei Starovoitov <[email protected]>

LeSeulArtichaut commented Aug 1, 2021

View reviewed changes

rust/kernel/miscdev.rs Outdated Show resolved Hide resolved

LeSeulArtichaut force-pushed the safety-comments branch from 2508227 to ba43c65 Compare August 1, 2021 00:47

LeSeulArtichaut requested a review from wedsonaf August 25, 2021 21:57

LeSeulArtichaut force-pushed the safety-comments branch from ba43c65 to 20af4c1 Compare November 2, 2021 14:08

wedsonaf reviewed Nov 2, 2021

View reviewed changes

ojeda reviewed Nov 2, 2021

View reviewed changes

bjorn3 reviewed Nov 4, 2021

View reviewed changes

rust/kernel/print.rs Outdated Show resolved Hide resolved

LeSeulArtichaut force-pushed the safety-comments branch from 41f6a48 to a87cb32 Compare November 4, 2021 11:13

wedsonaf approved these changes Nov 5, 2021

View reviewed changes

ojeda reviewed Nov 5, 2021

View reviewed changes

LeSeulArtichaut added 4 commits February 2, 2022 09:09

Add some missing // SAFETY comments

451b78b

Signed-off-by: Léo Lanteri Thauvin <[email protected]>

Address review comments

68821be

Document the safety requirements of rust_fmt_argument

f82aef7

Address review comments

feb20cf

LeSeulArtichaut force-pushed the safety-comments branch from a87cb32 to feb20cf Compare February 2, 2022 08:30

LeSeulArtichaut closed this Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some missing `// SAFETY` comments #475

Add some missing `// SAFETY` comments #475

LeSeulArtichaut commented Aug 1, 2021 •

edited

Loading

LeSeulArtichaut commented Nov 2, 2021

ojeda Nov 2, 2021 •

edited

Loading

LeSeulArtichaut Nov 3, 2021

ojeda Nov 5, 2021

LeSeulArtichaut Feb 2, 2022

Bevisy commented Sep 28, 2022

Add some missing // SAFETY comments #475

Add some missing // SAFETY comments #475

Conversation

LeSeulArtichaut commented Aug 1, 2021 • edited Loading

LeSeulArtichaut commented Nov 2, 2021

ojeda Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

LeSeulArtichaut Nov 3, 2021

Choose a reason for hiding this comment

ojeda Nov 5, 2021

Choose a reason for hiding this comment

LeSeulArtichaut Feb 2, 2022

Choose a reason for hiding this comment

Bevisy commented Sep 28, 2022

Add some missing `// SAFETY` comments #475

Add some missing `// SAFETY` comments #475

LeSeulArtichaut commented Aug 1, 2021 •

edited

Loading

ojeda Nov 2, 2021 •

edited

Loading