Page MenuHomeFreeBSD
Differential D49566

sys: add safe_read(9)
Needs ReviewPublic
Actions

Authored by kib on Mar 29 2025, 5:18 PM.
Tags
None
Referenced Files
F158041496: D49566.id178524.diff
Wed, May 27, 8:30 PM
F158035241: D49566.id.diff
Wed, May 27, 6:28 PM
F158029008: D49566.id178476.diff
Wed, May 27, 4:15 PM
Unknown Object (File)
Wed, May 27, 7:59 AM
Unknown Object (File)
Tue, May 26, 2:21 PM
Unknown Object (File)
Tue, May 26, 12:01 PM
Unknown Object (File)
Sat, May 23, 1:23 PM
Unknown Object (File)
Sat, May 2, 5:09 PM
View All 82 Files
Subscribers
aokblast
emaste
imp

Details

Reviewers
emaste
markj
jhb
Summary
amd64: extract uiomove_mem() from memrw()


The MD function with MI interface to provide a way to read arbitrary
(canonical) KVA.  amd64 only for now.

Tested by:      aokblast

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib created this revision.Mar 29 2025, 5:18 PM
Herald added a subscriber: imp. · View Herald TranscriptMar 29 2025, 5:18 PM
kib requested review of this revision.Mar 29 2025, 5:18 PM
kib mentioned this in D46193: sys: implement Kernel CFI from clang.Mar 29 2025, 5:20 PM
kib updated this revision to Diff 152842.Mar 29 2025, 5:37 PM

Slightly different version, avoid the thread-private flag.

emaste added a subscriber: emaste.Jul 30 2025, 11:00 PM
aokblast added a subscriber: aokblast.Sat, May 23, 2:49 PM
kib updated this revision to Diff 178476.Sat, May 23, 3:21 PM
kib retitled this revision from safe_read to sys: add safe_read(9).
kib edited the summary of this revision. (Show Details)
kib added reviewers: emaste, markj, jhb.
markj added inline comments.Sat, May 23, 5:40 PM
sys/amd64/amd64/trap.c
797

Maybe it should be formulated as if (__predict_false(!usermode && ...?

sys/sys/systm.h
557
559
kib marked 2 inline comments as done.Sat, May 23, 5:58 PM
kib added inline comments.
sys/amd64/amd64/trap.c
797

I believe that the first check for tf_rip makes the shortest path through the condition: userspace must work hard to fault with this specific %rip, and kernel most likely would not fault on safe_read_read with other conditions false.

kib updated this revision to Diff 178486.Sat, May 23, 5:58 PM

Edit the comment.

kib updated this revision to Diff 178524.Sun, May 24, 12:01 PM
kib edited the summary of this revision. (Show Details)

Complete overhaul. Stop modifying page fault handler at all. Reuse code from mem.c.

markj added inline comments.Tue, May 26, 2:28 PM
sys/amd64/amd64/machdep.c
1868 ↗(On Diff #178524)

uio_rw is indeed used. uio_segflg too, if we end up calling uiomove().

kib updated this revision to Diff 178621.Tue, May 26, 3:32 PM
kib marked an inline comment as done.

Fill uio_seg and uio_rw.

markj added inline comments.Tue, May 26, 3:55 PM
sys/amd64/amd64/machdep.c
1868 ↗(On Diff #178524)

IMO it would be better to explicitly initialize uio_td, either to curthread or NULL.

sys/amd64/amd64/uio_machdep.c
194 ↗(On Diff #178621)

So with this check and the DMAP boundary check above, we can only use safe_read() to access the kernel map and direct map. Is that the intent?

kib marked an inline comment as done.Tue, May 26, 4:34 PM
kib added inline comments.
sys/amd64/amd64/uio_machdep.c
194 ↗(On Diff #178621)

I believe it is enough for the use at hand, D46193.

More, returning to the 'faulting' implementation, I now think that it opened the access too wide, it should check at least that the address is in KVA. Of course we can return to the 'faulting' way of doing things for other addresses which cannot be handled by pmap_extract(kernel_pmap). I thought that avoiding adding a check to the page fault path is good.

kib updated this revision to Diff 178633.Tue, May 26, 4:35 PM

Initialize uio_td.

Revision Contents
Changeset List

PathSize
sys/
amd64/
amd64/
20 lines
9 lines
sys/
8 lines

Diff 178476

View Options

sys/amd64/amd64/support.S

Loading...
View Options

sys/amd64/amd64/trap.c

Loading...
View Options

sys/sys/systm.h

Loading...