tests: kern: add some tests for TIOCSTI
ClosedPublic
Actions

Authored by kevans on Sat, May 24, 4:12 AM.

Details

Reviewers

emaste
kib
markj

Commits

rGd094dd9071ce: tests: kern: add some tests for TIOCSTI

Summary

These offer at least rudimentary coverage of TIOCSTI, ensuring that it
basically works and does what it's described to do and throws errors for
unprivileged use that is supposed to be blocked.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kevans created this revision.Sat, May 24, 4:12 AM

Herald added a subscriber: imp. · View Herald TranscriptSat, May 24, 4:12 AM

kevans requested review of this revision.Sat, May 24, 4:12 AM

Harbormaster completed remote builds in B64404: Diff 155967.Sat, May 24, 4:12 AM

kevans added a parent revision: D50506: kern: tty: refactor TIOCSTI privilege checks slightly.Sat, May 24, 4:13 AM

kib added inline comments.Sat, May 24, 8:33 AM

tests/sys/kern/tty/test_sti.c
38
167
179	why not memcmp?
197
219	again memcmp?
338	Did you considered having a shared mapping from where the parent would pass the data to child? You would still need a sync to reliably get the data consistent, but at least setup would not require the testing element to participate as the data channel. I do not insist on the design change. It only looks un-natural to me.

quentin.thebault_defenso.fr added a subscriber: quentin.thebault_defenso.fr.Sat, May 24, 2:09 PM

kevans added inline comments.Sat, May 24, 2:15 PM

tests/sys/kern/tty/test_sti.c
338	I hadn't thoroughly considered alternatives here, no- I can rewrite it to use a shared mapping. What I would've really liked was a way to just reopen the underlying file for STDIN_FILENO with different access rights, but I didn't want to introduce a dependency on fdescfs (and looking at that path, it doesn't look like you could reopen it with different rights anyways).

kib added inline comments.Sat, May 24, 2:20 PM

tests/sys/kern/tty/test_sti.c
338	Hm, are you suggesting that adding O_PATH support to dup3(2) is useful?

kevans added inline comments.Sat, May 24, 2:35 PM

tests/sys/kern/tty/test_sti.c
338	Not quite, I don't think, because I still need ioctl(2) support and there's no way to get from an O_PATH fd back to a tty that's been downgraded to read or write only. What would have been nice is if openat(2) could take a non-dirfd as long as the path is "." to open a new file for the underlying vnode, but I don't think I could get agreement on that kind of thing.

kib added inline comments.Sat, May 24, 2:36 PM

tests/sys/kern/tty/test_sti.c
338	D50511 is more generic. You dup() your file, then convert it to O_PATH with F_TO_O_PATH, then reopen it by openat(EMPTYPATH) with the needed permissions.

kevans added inline comments.Sat, May 24, 3:39 PM

tests/sys/kern/tty/test_sti.c
338	A-ha, I missed O_EMPTY_PATH. I drafted up a PoC with the current setup but opening up an O_PATH fd from `readbuf` first and using that for the others and the result looks good to me; moving over to D50511.

Address review comments, rebase on top of proposed F_DUP_OPATH

Harbormaster completed remote builds in B64412: Diff 155993.Sun, May 25, 12:59 AM

kib accepted this revision.Sun, May 25, 5:53 AM

kib added inline comments.

tests/sys/kern/tty/test_sti.c
96

This revision is now accepted and ready to land.Sun, May 25, 5:53 AM

kib added inline comments.Sun, May 25, 6:03 AM

tests/sys/kern/tty/test_sti.c
297	In fact, I wonder, could you please try pfd = openat(STDIN_FILENO, "", O_EMPTY_PATH \| O_PATH); instead of F_DUP_OPATH. I do not see why wouldn't it work, and then F_DUP_OPATH is simply not needed.