In D60408#1385683, @olce wrote:

In D60408#1385675, @dteske wrote:

Right, given the high level of fallout from what can happen if an attacker gains access to Dtrace/dwatch on a system without being prompted for credentials (but instead merely supposing that remote access control is sufficient alone) additional scrutiny is required. The change is fine for systems that roundly reject mac_do for their inability to properly gate access in the face of failed safety systems.

You don't need to "reject" anything. By default, mac_do(4) does not allow anything (assuming it's even loaded, which it should not unless you do something explicit).

offset.inc was a huge mistake and should die in a fire, but it's used can't be killed easily.

In D60408#1385687, @olce wrote:

In D60408#1385675, @dteske wrote:

I don't recall who I talked with at BSDCan, but we were in the hacker lounge as he was porting the code from NetBSD to add prompting, and before the convention was over he even had demonstrated a portion of working code.

I haven't seen any code, so can only speculate, but I wouldn't be surprised if that hacker did not understand the current architecture and was using a setuid mdo(1) executable, which is against the purpose of mdo(1). But if that's not the case, I'd like to see this code.

In D60408#1385675, @dteske wrote:

I don't recall who I talked with at BSDCan, but we were in the hacker lounge as he was porting the code from NetBSD to add prompting, and before the convention was over he even had demonstrated a portion of working code.

Calculate mp by VOP_GETWRITEMOUNT()
Do not leak VI_FOPENING

In D60408#1385675, @dteske wrote:

Right, given the high level of fallout from what can happen if an attacker gains access to Dtrace/dwatch on a system without being prompted for credentials (but instead merely supposing that remote access control is sufficient alone) additional scrutiny is required. The change is fine for systems that roundly reject mac_do for their inability to properly gate access in the face of failed safety systems.

I do approve of this change, btw.

Sorry, withdrawing temporarily again, it looks like there is only a single PowerPC platform that does not have the BSP == CPUID 0 invariant, so going to propose a fix for that, which should lead to a lot of simplification of this revision.

In D60408#1385618, @olce wrote:

In D60408#1385614, @dteske wrote:

mac_do is still not ready for corporate use, because it has a security flaw that I discussed with others at BSDCan.

I'm quite surprised by this statement, on multiple respects. First, I've never been informed of that, although I'm the maintainer. Second, I don't think this is true at all. mac_do(4) is perfectly usable in a corporate use, and has been developed with the highest quality in mind. You have to understand what it can and cannot do, though.

In D60274#1385664, @wanpengqian_gmail.com wrote:

Thanks for the review. Agreed on all three points. I'm new to the process here, so two questions on how you'd like them delivered:

1. EHB handling: I'd follow the spec (and QEMU's model): when EHB is already set, only post the event to the ring and don't touch IP/EINT or raise an interrupt; when a write to ERDP clears EHB and the ring is not empty, set IP/EHB again and raise. IMOD pacing (and the IMODI reset value) would stay for a separate change. Would you prefer the EHB part as a separate parent revision with this INTx change rebased on top, or folded into this revision as one commit?

2. The IMAN_IP/IMAN_IE rename touches sys/dev/usb/controller/xhcireg.h, which xhci(4) uses too. I'm happy to do it as a follow-up revision once this one lands, if that works for you.

The pci_msi_enabled() check in pci_xhci_deassert_interrupt() will be in the next update.

In D60187#1382420, @mhorne wrote:

This one requires a bit more study on my part...

• Implement fernape@ suggestion.

Thanks for the review. Agreed on all three points. I'm new to the process here, so two questions on how you'd like them delivered:

Address review feedback:

In D60404#1385645, @glebius wrote:

I'm not a voting srcmgr member, so not putting my approval here.

The change is correct, but as Pouria noted all the stuff around here is fragile. There could be bugs fixing other bugs :) Let's proceed with this change and see if there is any fallout that needs to be dealt with.

Thanks!

• Fix security issue spotted by @markj in previous revision.
• Reduce diff (@olce).
• Implement BSD .sched_policy to Linux .sched_policy mapping.

I'm not a voting srcmgr member, so not putting my approval here.

There was a period of time when Mr. Macy was committing a lot of his draft ideas right into svn HEAD. Most of them were never finished or continued. This is one of them.

In D60397#1385509, @markj wrote:

Do you have any script which could be used to verify the linker script after future imports?

This is a pre-existing problem, but it might be nice to rername these at some point to IMAN_IP and IMAN_IE to line up witih the spec.

In D60408#1385614, @dteske wrote:

mac_do is still not ready for corporate use, because it has a security flaw that I discussed with others at BSDCan.

If I understand correctly (asking for confirmation of principles), if you do not configure mac_do, that it does not use it, correct?

In passing, I noticed that <sys/kpilite.h> is not used in the tree (and only was for a couple of weeks). Was wondering if we should remove it. Doing so, however, removes a capability (that is not used, and could be restored later), so not too sure about it.

In D60368#1384760, @ngie wrote:

For the record: this is why code changes for utilities like this need tests written beforehand (@des has been leading by example in this area).

i like it. @jhibbits what do you think?

So please fix the bug pointed out by Mark, and I'll review the change again.

I was trying to do the same thing with PROTO_IF + SI_ORDER_FIRST on VNET_SYSUNINIT. But this version looks better to me. of course it depends on if_epair.

I tried that once, there was a problem with it, but I don't remember anymore...
But that was about 5 months ago, it might be ok now.

Abandoned in favor of https://reviews.freebsd.org/D60407

Also posted to freebsd-net@ for wider review.

• Change order of copyright and SPDX tag.