On 26/03/2019 09:08, Jan Beulich wrote: >> >>>> Leave the warning which identifies the problematic devices, but drop the >>>> remaining logic. This leaves the system in better overall state, and >>>> working >>>> in the same way that it did in previous releases. >>> I wonder whether you've taken the time to look at the description >>> of the commit first introducing this logic (a8059ffced "VT-d: improve >>> RMRR validity checking"). I find it worrying in particular to >>> effectively revert a change which claims 'to avoid any security >>> vulnerability with malicious s/s re-enabling "supposed disabled" >>> devices' without any discussion of why that may have been a >>> wrong perspective to take. >> I had, and as a maintainer, I'd reject a patch like that were it >> presented today. > Understood. But whether you'd accept it with a better description > is unknown, I assume.
I severely doubt I'd accept it at all, because it is entirely unreasonable behaviour. At best, it is the equivalent of throwing your hands up in the air and saying "I give up", and that is not good enough behaviour for Xen. > >> There is a nebulous claim of security, but it is exactly that - >> nebulous. There isn't enough information to work out what the concern >> was, and even if the concern was valid, disabling VT-d across the system >> isn't an appropriate action to take. > This heavily depends on the position the system's admin takes: > Enabling VT-d in an incomplete fashion may as well be considered > worse than not enabling it at all. No - that's simply not true, or a reasonable position to take. Disabling the IOMMU prevents the system from booting with a PVH dom0. I am not aware of a credible case where partially enabled VT-d is less secure than no VT-d, and there is one headline case now where disabled VT-d causes a failure to boot. > Furthermore, as much as the security related claim there is > nebulous, your description - I'm sorry to say that - isn't much > better, as you don't clarify why there's _no_ security aspect > there. Stating that "this leaves the system in better overall > state" without making clear why that is _for everyone_ is not > helpful at all. The nebulous security claim is not relevant to this patch. This code was not run previously. An unexpected consequence of a change in 4.12 caused it to run, and break booting on some (sadly rather common) systems. This is a regression in 4.12 and needs resolving. The choice is between reverting dcf41790 or removing this code, and reverting dcf41790 is obviously not a valid thing to do. Beyond that, I really don't care what the exact behaviour of 4.11 was. If there is a real security issue then it still needs fixing on all versions of Xen, and this change doesn't alter that property. However, until someone can work out what the alleged issue is, we can't really progress this argument, and we mustn't keep broken code simply because it purports to "fix" an unspecified issue. > >> I'm not sure what more you are looking for, but this is very clear cut >> and safe from my point of view. > Well, your claim regarding "4.11 and earlier" is clearly wrong I have made a statement, backed up with specific reference to the code which, to the best of my ability, demonstrates it to be true. If you believe contrary then clearly identify the fault in my reasoning. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
