[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] VT-d/RMRR: Avoid memory corruption in add_user_rmrr()
(adding VT-d maintainers to Cc) >>> On 31.01.17 at 18:00, <venu.busireddy@xxxxxxxxxx> wrote: > On Tue, Jan 31, 2017 at 08:56:09AM -0700, Jan Beulich wrote: >> >>> On 31.01.17 at 16:22, <venu.busireddy@xxxxxxxxxx> wrote: >> > On Tue, Jan 31, 2017 at 02:55:50AM -0700, Jan Beulich wrote: >> >> >>> On 30.01.17 at 22:09, <venu.busireddy@xxxxxxxxxx> wrote: >> >> > On Mon, Jan 30, 2017 at 03:39:23AM -0700, Jan Beulich wrote: >> >> >> I notice, however, that register_one_rmrr() returning success >> >> >> doesn't always mean success, so in non-debug builds we may be >> >> >> left without any log message here despite there being a problem >> >> >> with what the user specified. Elena, Venu, can you look into this >> >> >> please? Perhaps the function should return a positive value in >> >> >> that case, so that the original caller can retain its current behavior >> >> >> but the newly added caller can be adjusted? >> >> > >> >> > As far as I can see, register_one_rmrr() can only return success even >> >> > when there is an error is when the user specifies a bad device (and >> >> > pci_device_detect() fails), and we set "ignore" to true. Given that, >> >> > the simplest way to fix this would be to return -EINVAL in the "if ( >> >> > ignore )" block. Do you think that would be acceptable? If you agree, >> >> > I will send the patch for review. >> >> >> >> No, as said, the other caller of register_one_rmrr() needs to retain >> >> its current behavior. >> > >> > I sent you another email explaining why maintaining the current behavior >> > for the old caller will be difficult *without* making any changes in >> > that path also. Waiting for your reply for that email. >> >> I don't think I've seen that, yet. > > Here is the justification for returning -EINVAL. > > Whether we return a positive value or negative value here, the net result > in acpi_parse_dmar() will be that VT-d will be disabled. The advantage > of returning -EINVAL is that no other changes will be needed. The > disadvantage is that VT-d will be disabled for this error also. This disadvantage is what counts. And returning a positive value was of course meant to be accompanied by an adjustment to the pre-existing caller (to check just for negative values instead of for non-zero ones). That is clearly better (going forward) than to have this caller special case -EINVAL. > That brings up this question. Do we want to disable VT-d if the PCIe > device specified via ACPI cannot be detected? We do so if the address > range is incorrectly specified. If the answer is yes, then returning > -EINVAL may be acceptable, and that will be the only change needed. If > not, we will have to make changes to acpi_parse_dmar() also to deal > with the non-zero return value from register_one_rmrr() for failure to > detect the device. Part of the issue is that you appear to think only in terms of a single device. The logic, however, is such that initialization failure is being avoided as long as at least one of the (possibly many) listed devices can be successfully probed. > That brings up one another question. In the original code, is > there a historical reason why the ckeck on rmrru->base_address and > rmrru->end_address warranted returning -EFAULT, thus diabling the VT-d, > but failure to detect the device did not warrant returning an error? For the very reason of there possibly being multiple devices. I would guess that some firmware authors aren't pedantic enough to e.g. avoid listing devices which are present depending on some configuration option. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |