Skip to content

xen_4_8: init at 4.8.1#26492

Merged
joachifm merged 2 commits intoNixOS:masterfrom
michalpalka:new-xen
Jun 30, 2017
Merged

xen_4_8: init at 4.8.1#26492
joachifm merged 2 commits intoNixOS:masterfrom
michalpalka:new-xen

Conversation

@michalpalka
Copy link
Copy Markdown

@michalpalka michalpalka commented Jun 9, 2017

This commit adds the xen_4_8 package to be used instead of
xen (currently at 4.5.5):

  • Add packages xen_4_8, xen_4_8-slim and xen_4_8-light
  • Add packages qemu_xen_4_8 and qemu_xen_4_8-light to be used
    with xen_4_8-slim and xen_4_8-light respectively.
  • Add systemd to buildInputs of xen (it is required by oxenstored)
  • Adapt xen service to work with the new version of xen
  • Use xen-init-dom0 to initlilise dom0 in xen-store
  • Currently, the virtualisation.xen.stored option is ignored
    if xen 4.8 is used
Motivation for this change

This introduces Xen 4.8.1, which is the most current version, along with a few needed changes and a few fixes.

To use the new xen, use these configuration options.

virtualisation.xen.package = pkgs.xen_4_8;
virtualisation.xen.qemu-package = pkgs.xen_4_8;
virtualisation.xen.qemu = "${pkgs.xen_4_8}/lib/xen/bin/qemu-system-i386";

xen_4_8-slim may be used with this configuration:

virtualisation.xen.package = pkgs.xen_4_8-slim;
virtualisation.xen.qemu-package = pkgs.qemu_xen_4_8;
virtualisation.xen.qemu = "${pkgs.qemu_xen_4_8}/bin/qemu-system-i386";

Finally, xen_4_8-light may be used with this configuration:

virtualisation.xen.package = pkgs.xen_4_8-light;
virtualisation.xen.qemu-package = pkgs.qemu_xen_4_8-light;
virtualisation.xen.qemu = "${pkgs.qemu_xen_4_8-light}/bin/qemu-system-i386";

One notable limitation of this PR is that the virtualisation.xen.stored is ignored with xen 4.8, which I hope to address in the future.

Furthermore, there are a few statements in xen-dom0.nix and generic.nix that are conditional on the xen version, which is not ideal. Please let me know if you would like it to be handled differently.

Also, the package refers to the fork https://github.com/michalpalka/xen-libhvm of the xen-libhvm repo, which needed to be adapted to work with xen 4.8. This might equally be solved using a patch.

Please let me know if you have any feedback.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 seconds sounds like a lot to me for a message processed by systemd. Is there something else, which slows down the process?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to pick the right value here, as the call to sd_notify() may be delayed by an arbitrary amount of time. Probably 1 second would be enough, but I chose 10 just to be on a safe side. Please note that this delay will not delay the boot process, as systemd will go ahead and start other units as soon as it gets the notification. So the only effect of a longer delay will be the shell script running for some time afterwards. On the other hand, if the notification is lost then the startup of the unit will hang for a longer time until it gets timed out by systemd and fails.

This is an ugly workaround–I believe that you can solve it properly by using MAINPID= together with sd_notify(). I will submit a bug report about it to xen.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sleep 10 is fragile, as pointed out by others. If it cannot be done in any other way, leave it like this, but if it can be fixed, fix it. What happened to the bug report to Xen?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this code has since been dropped from the PR–see comment below, which I reproduce here:

Interestingly, as I just found out, the patch is not needed anymore as I cannot reproduce the problematic behaviour with systemd 233. It seems that there was a problem with systemd 232, which led to the notification being dropped. Given that, it seems that the scripts in xen are correct, and the patch can be dropped–I'll drop it from the commit.

@oxij
Copy link
Copy Markdown
Member

oxij commented Jun 12, 2017 via email

@michalpalka
Copy link
Copy Markdown
Author

Hi!

Problems I see without testing:

  • You introduce tabs in xen-dom0.nix (and I can now assume your tabstop = 8 :) )

That's an oversight–I'll fix that and amend the commit

  • The systemd patch is ugly. It potentially slows down complete system
    boot by 10s. Is it upstream somewhere? Can we get it from upstream?
    If not, how do they solve the same problem upstream?

It does not slow the boot by 10s, because systemd continues as soon as it gets the notification from oxenstored. Interestingly, as I just found out, the patch is not needed anymore as I cannot reproduce the problematic behaviour with systemd 233. It seems that there was a problem with systemd 232, which led to the notification being dropped. Given that, it seems that the scripts in xen are correct, and the patch can be dropped–I'll drop it from the commit.

  • Can 4.8's oxenstored build without systemd dependency? (A problem for
    SLNOS which uses s6 init).

Yes, oxenstored was previously built without an explicit systemd dependency. As a result, systemd was included when the xen package was compiled, but not xen-slim or xen-light. However, if oxenstored is built without systemd, it cannot be used with systemd services, so systemd needs to be somehow included conditionally.

@michalpalka
Copy link
Copy Markdown
Author

I have just amended the commit removing the tabs and the race condition workaround. I have not done anything about the systemd dependency, and I am not sure how to continue. Should the packages have a useSystemd flag?

@oxij
Copy link
Copy Markdown
Member

oxij commented Jun 14, 2017 via email

@michalpalka
Copy link
Copy Markdown
Author

michalpalka commented Jun 14, 2017

Eh? Why? Systemd wasn't even referenced, IIRC.

It seems that systemd was propagated from one of the other dependencies, and just picked up by the configure script.

@joachifm joachifm added the 8.has: package (new) This PR adds a new package label Jun 16, 2017
@oxij
Copy link
Copy Markdown
Member

oxij commented Jun 25, 2017 via email

@michalpalka
Copy link
Copy Markdown
Author

I would like to add security patches against this version of xen. Should I amend this PR, or create a new one?

@oxij
Copy link
Copy Markdown
Member

oxij commented Jun 26, 2017 via email

@michalpalka
Copy link
Copy Markdown
Author

michalpalka commented Jun 26, 2017

I have added a commit with security patches, and rebased everything.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not hardcode /var/log/xen. As a default it's fine.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code that you are referring to has been carried over from the service that was created for xen 4.5 (or earlier). An argument for keeping it is to try to minimise the change. If you insist, then I can remove it, but otherwise we may keep it as is and drop it when we drop xen 4.5.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the rest of the module, /var/log/xen seems to be hardcoded in a lot of other places. If you want to get rid of it, then I suggest to do it with a separate PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constant "4.8" is repeated multiple times.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you see a named constant here instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just to make sure people don't make search and replace errors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is important, create an issue for this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also carried over from the previous package. I'm not sure if I understand this TODO. Is the idea to use ipxe packaged separately?

@oxij
Copy link
Copy Markdown
Member

oxij commented Jun 27, 2017 via email

@michalpalka
Copy link
Copy Markdown
Author

michalpalka commented Jun 27, 2017

Is the idea to use ipxe packaged separately?

Yes, to make xen-light even lighter.

I'll create an issue.

Actually, this code has since been dropped from the PR–see comment below, which I reproduce here:

No, I just fetched this branch and its still there.

It seems that I have dropped the reference to the patch, but not the patch itself–sorry for that, I will fix it.

Also the tabs in xen-dom0.nix are still there.

That's right–I'll fix it too.

Michał Pałka added 2 commits June 27, 2017 12:01
This commit adds the xen_4_8 package to be used instead of
xen (currently at 4.5.5):
 * Add packages xen_4_8, xen_4_8-slim and xen_4_8-light
 * Add packages qemu_xen_4_8 and qemu_xen_4_8-light to be used
   with xen_4_8-slim and xen_4_8-light respectively.
 * Add systemd to buildInputs of xen (it is required by oxenstored)
 * Adapt xen service to work with the new version of xen
 * Use xen-init-dom0 to initlilise dom0 in xen-store
 * Currently, the virtualisation.xen.stored option is ignored
   if xen 4.8 is used
…4.8)

This commit contains security patches for xen 4.8. The patches
for XSA-216 applied to the kernel are omitted, as they are part of
80e0cda.

XSA-216 Issue Description:

> The block interface response structure has some discontiguous fields.
> Certain backends populate the structure fields of an otherwise
> uninitialized instance of this structure on their stacks, leaking
> data through the (internal or trailing) padding field.

More: https://xenbits.xen.org/xsa/advisory-216.html

XSA-217 Issue Description:

> Domains controlling other domains are permitted to map pages owned by
> the domain being controlled.  If the controlling domain unmaps such a
> page without flushing the TLB, and if soon after the domain being
> controlled transfers this page to another PV domain (via
> GNTTABOP_transfer or, indirectly, XENMEM_exchange), and that third
> domain uses the page as a page table, the controlling domain will have
> write access to a live page table until the applicable TLB entry is
> flushed or evicted.  Note that the domain being controlled is
> necessarily HVM, while the controlling domain is PV.

More: https://xenbits.xen.org/xsa/advisory-217.html

XSA-218 Issue Description:

> We have discovered two bugs in the code unmapping grant references.
>
> * When a grant had been mapped twice by a backend domain, and then
> unmapped by two concurrent unmap calls, the frontend may be informed
> that the page had no further mappings when the first call completed rather
> than when the second call completed.
>
> * A race triggerable by an unprivileged guest could cause a grant
> maptrack entry for grants to be "freed" twice.  The ultimate effect of
> this would be for maptrack entries for a single domain to be re-used.

More: https://xenbits.xen.org/xsa/advisory-218.html

XSA-219 Issue Description:

> When using shadow paging, writes to guest pagetables must be trapped and
> emulated, so the shadows can be suitably adjusted as well.
>
> When emulating the write, Xen maps the guests pagetable(s) to make the final
> adjustment and leave the guest's view of its state consistent.
>
> However, when mapping the frame, Xen drops the page reference before
> performing the write.  This is a race window where the underlying frame can
> change ownership.
>
> One possible attack scenario is for the frame to change ownership and to be
> inserted into a PV guest's pagetables.  At that point, the emulated write will
> be an unaudited modification to the PV pagetables whose value is under guest
> control.

More: https://xenbits.xen.org/xsa/advisory-219.html

XSA-220 Issue Description:

> Memory Protection Extensions (MPX) and Protection Key (PKU) are features in
> newer processors, whose state is intended to be per-thread and context
> switched along with all other XSAVE state.
>
> Xen's vCPU context switch code would save and restore the state only
> if the guest had set the relevant XSTATE enable bits.  However,
> surprisingly, the use of these features is not dependent (PKU) or may
> not be dependent (MPX) on having the relevant XSTATE bits enabled.
>
> VMs which use MPX or PKU, and context switch the state manually rather
> than via XSAVE, will have the state leak between vCPUs (possibly,
> between vCPUs in different guests).  This in turn corrupts state in
> the destination vCPU, and hence may lead to weakened protections
>
> Experimentally, MPX appears not to make any interaction with BND*
> state if BNDCFGS.EN is set but XCR0.BND{CSR,REGS} are clear.  However,
> the SDM is not clear in this case; therefore MPX is included in this
> advisory as a precaution.

More: https://xenbits.xen.org/xsa/advisory-220.html

XSA-221 Issue Description:

> When polling event channels, in general arbitrary port numbers can be
> specified.  Specifically, there is no requirement that a polled event
> channel ports has ever been created.  When the code was generalised
> from an earlier implementation, introducing some intermediate
> pointers, a check should have been made that these intermediate
> pointers are non-NULL.  However, that check was omitted.

More: https://xenbits.xen.org/xsa/advisory-221.html

XSA-222 Issue Description:

> Certain actions require removing pages from a guest's P2M
> (Physical-to-Machine) mapping.  When large pages are in use to map
> guest pages in the 2nd-stage page tables, such a removal operation may
> incur a memory allocation (to replace a large mapping with individual
> smaller ones).  If this allocation fails, these errors are ignored by
> the callers, which would then continue and (for example) free the
> referenced page for reuse.  This leaves the guest with a mapping to a
> page it shouldn't have access to.
>
> The allocation involved comes from a separate pool of memory created
> when the domain is created; under normal operating conditions it never
> fails, but a malicious guest may be able to engineer situations where
> this pool is exhausted.

More: https://xenbits.xen.org/xsa/advisory-222.html

XSA-224 Issue Description:

> We have discovered a number of bugs in the code mapping and unmapping
> grant references.
>
> * If a grant is mapped with both the GNTMAP_device_map and
> GNTMAP_host_map flags, but unmapped only with host_map, the device_map
> portion remains but the page reference counts are lowered as though it
> had been removed. This bug can be leveraged cause a page's reference
> counts and type counts to fall to zero while retaining writeable
> mappings to the page.
>
> * Under some specific conditions, if a grant is mapped with both the
> GNTMAP_device_map and GNTMAP_host_map flags, the operation may not
> grab sufficient type counts.  When the grant is then unmapped, the
> type count will be erroneously reduced.  This bug can be leveraged
> cause a page's reference counts and type counts to fall to zero while
> retaining writeable mappings to the page.
>
> * When a grant reference is given to an MMIO region (as opposed to a
> normal guest page), if the grant is mapped with only the
> GNTMAP_device_map flag set, a mapping is created at host_addr anyway.
> This does *not* cause reference counts to change, but there will be no
> record of this mapping, so it will not be considered when reporting
> whether the grant is still in use.

More: https://xenbits.xen.org/xsa/advisory-224.html
@michalpalka
Copy link
Copy Markdown
Author

I have pushed new versions of commits, which hopefully address all concerns.

It seems that I have dropped the reference to the patch, but not the patch itself–sorry for that, I will fix it.

Actually, the reference to the patch was also there. I must have pushed old (unamended) commits by mistake. Thanks for the vigilance.

@joachifm joachifm merged commit a8ba50d into NixOS:master Jun 30, 2017
@michalpalka
Copy link
Copy Markdown
Author

Thanks a lot @Mic92 @oxij @0xABAB @joachifm for reviewing and merging!

It would be great if some of you could also take a look at #25724, which adds pvgrub that is useful to have with xen.

@michalpalka michalpalka deleted the new-xen branch July 1, 2017 22:27
@SigmaSquadron SigmaSquadron added the 6.topic: xen-project Issues and PRs related to the Xen Project Hypervisor. label Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: xen-project Issues and PRs related to the Xen Project Hypervisor. 8.has: package (new) This PR adds a new package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants