Re: [Xen-devel] [PATCH] VT-d/RMRR: Avoid memory corruption in add_user_rmrr()

["Jan Beulich" <JBeulich@xxxxxxxx>]

(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