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

[Venu Busireddy <venu.busireddy@xxxxxxxxxx>]

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.

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.

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?

> >> Also - any reason you've dropped xen-devel and Andrew?
> > 
> > Thought this would avoid email churn. Once we agree how you want the
> > final solution looks like, I will send out the patch to everyone that is
> > needed to be copied. But if the standard process is to reply to everyone
> > in every email, I will follow that in the future. Please confirm.
> 
> Yes, discussions like this should be held in public, both for others
> to chime int as well as for it getting archived (e.g. for future
> archaeologists).

Understood. Hence I am copying my personal reply to you (with minor
changes) to everyone involved.

Venu


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel