New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xen 48 #16

Closed
wants to merge 6 commits into
base: xen-48
from

Conversation

Projects
None yet
3 participants
@sarahn

sarahn commented Jun 9, 2017

This appears to work with selinux on and centos 6. After installing xen-debuginfo, I built a patch for xsa214 from rpmbuild/BUILD/xen-4.8.1 using:

/usr/bin/livepatch-build -s . -c xen/.config -p ~/xsa214.patch -o xsa214 --xen-syms /usr/lib/debug/xen-syms-4.8.1-1.el6 --depends $(eu-readelf -n /usr/lib/debug/xen-syms-4.8.1-1.el6 | tail -1 | sed 's/.*Build ID: //')

The instructions on https://wiki.xen.org/wiki/LivePatch for uploading and applying the patch appeared to work:

$ sudo xl info | grep build_id
build_id : b249a7b99119d45e743846980f5895f9adb15edc

$ sudo /usr/sbin/xen-livepatch upload xsa214 xsa214.livepatch
Uploading xsa214.livepatch (523280 bytes)
$ sudo /usr/sbin/xen-livepatch list
ID | status
----------------------------------------+------------
xsa214 | CHECKED

$ sudo xl dmesg | tail -15
(XEN) 'x' pressed - Dumping all livepatch patches
(XEN) build-id: b249a7b99119d45e743846980f5895f9adb15edc
(XEN) name=xsa214 state=CHECKED(1) ffff82d080409000 (.data=ffff82d08040a000, .rodata=ffff82d08040a000) using 2 pages.
(XEN) steal_page patch ffff82d08017d820(528) with ffff82d0804090d0 (565)
(XEN) build-id=6bf60663b32d6767270a75e3a8816ad5f1fe5836
(XEN) depend-on=b249a7b99119d45e743846980f5895f9adb15edc

$ sudo /usr/sbin/xen-livepatch apply xsa214
Performing apply:. completed
$ sudo /usr/sbin/xen-livepatch list
ID | status
----------------------------------------+------------
xsa214 | APPLIED

@gwd

gwd requested changes Jun 9, 2017 edited

Enabling livepatch is fine, but this way seems a bit hacky to me.

What about modifying the build section to create a .config file like this:

#if %{with_livepatch}
echo "CONFIG_LIVEPATCH=y" > xen/.config
// and so on
#endif

make -C xen olddefconfig
@gwd

Everything looks good Sarah, except for two things:

  • A commit message detailing the main changes (use source.cfg, check sources)

  • This introduces some new code so I think it needs a Signed-off-by.

BTW I don't mind the sig checking, but here I don't think it actually adds much security. For one, https is already doing a certain amount of sig checking; for two, since you download the sig just after downloading the source, all you're verifying is that if there was a MITM attacker, they remembered to change the sig to match their modified binary. Given that in order to do a MITM they would have had to get a fake certificate for xenproject.org, they're likely to have their stuff together.

If we really wanted to make the sig checking more secure than plain https, we'd check the signatures for the files into this repo, so that anyone doing a build would have two independent routes that would need to be hijacked. (And I think hijacking git without being noticed is significantly more difficult than hijacking a single set of signed files on a website.)

But that's just an idea for the future; as I said, the signature checking code is fine as it is for now.

@gwd

This will cause it to fail when built in the CBS. You're getting this because there must be some package you have that the core system doesn't, that configure is detecting when building.

Have you tried building with mock?

@gwd

This comment has been minimized.

Show comment
Hide comment
@gwd

gwd Jun 9, 2017

Member

Thanks for this series, Sarah! One other comment -- you need to bump the Release number when submitting changes that may require a rebuild. Also, you need to add changes to the changelog at the bottom of the .spec file.

Member

gwd commented Jun 9, 2017

Thanks for this series, Sarah! One other comment -- you need to bump the Release number when submitting changes that may require a rebuild. Also, you need to add changes to the changelog at the bottom of the .spec file.

@sarahn

This comment has been minimized.

Show comment
Hide comment
@sarahn

sarahn Jun 10, 2017

I think I made the changes you requested.

sarahn commented Jun 10, 2017

I think I made the changes you requested.

Sarah Newman
Incorporate source.cfg and check signature for xen if key present in …
…gpg keyring

Signed-off-by: Sarah Newman <srn@prgmr.com>
@gwd

gwd approved these changes Jun 13, 2017

This looks better, thanks.

I have some suggestions which I won't require, but since you'll probably be re-submitting anyway (since I have comments on the other patches), you might consider making (or not, totally your choice):

  1. Use >> instead of >
  2. Move the make -C xen oldconfig outside of the %if block.

That way, if there are other things we want to be able to optionally enable or disable in the hypervisor, we can have a long string of "appends" to .config, followed by a single make olddefconfig.

Show outdated Hide outdated SPECS/xen.spec
@gwd

gwd approved these changes Jun 13, 2017

@gwd

xenpolicy is useless if flask isn't enabled (which it won't be by default); there's no point in shipping a policy that can't be used by anything.

Two options:

  1. Add --disable-xsmpolicy to the main configure (around like 418)
  2. Add rm -f %{buildroot}/boot/xenpolicy-* somewhere under the "kill unwanted stuff" comment.

I prefer the first because it avoids spending time building the policy in the first place, but either would be OK.

One other comment: On a system that didn't include checkpolicy by default (such as the CBS), introducing this in this patch and the checkpolicy requirement in another patch opens up a "window" where the build will fail. If adding a checkpolicy dependency was the right solution, then it should have been added in the same commit which added the resulting additional file.

Thanks for your work!

@gwd

gwd approved these changes Jun 13, 2017

Sarah Newman added some commits Jun 14, 2017

@sarahn

This comment has been minimized.

Show comment
Hide comment
@sarahn

sarahn Jun 14, 2017

added --disable-xsmpolicy

sarahn commented Jun 14, 2017

added --disable-xsmpolicy

Sarah Newman added some commits Jun 9, 2017

Sarah Newman
Enable live patching by default. Disable with --without livepatch
Signed-off-by: Sarah Newman <srn@prgmr.com>
Sarah Newman
Add livepatch-build-tools to build
Signed-off-by: Sarah Newman <srn@prgmr.com>
Sarah Newman
Apply XSAs 213-214 and bump release number
Signed-off-by: Sarah Newman <srn@prgmr.com>
@sarahn

This comment has been minimized.

Show comment
Hide comment
@sarahn

sarahn Jun 14, 2017

Moved make -C xen olddefconfig outside %if

sarahn commented Jun 14, 2017

Moved make -C xen olddefconfig outside %if

wget -P SOURCES/ $XEN_RELEASE_BASE/$XEN_VERSION/$XEN_RELEASE_FILE.sig || exit 1
fi
gpg --status-fd 1 --verify SOURCES/$XEN_RELEASE_FILE.sig SOURCES/$XEN_RELEASE_FILE \
| grep -q "GOODSIG ${XEN_KEY}" || exit 1

This comment has been minimized.

@sheep

sheep Jun 16, 2017

Contributor

That's look good, but I think we could go one step further, and use the fingerprint of the key instead of the shorter "long keyid".

So having XEN_KEY=23E3222C145F4475FA8060A783FE14C957E82BD9
then, grep for "^\[GNUPG:\] VALIDSIG ${XEN_KEY}"

From GnuPG documentation, status lines always start with "[GNUPG:] ", and VALIDSIG is the same in this case as GOODSIG, but for fingerprint.

@sheep

sheep Jun 16, 2017

Contributor

That's look good, but I think we could go one step further, and use the fingerprint of the key instead of the shorter "long keyid".

So having XEN_KEY=23E3222C145F4475FA8060A783FE14C957E82BD9
then, grep for "^\[GNUPG:\] VALIDSIG ${XEN_KEY}"

From GnuPG documentation, status lines always start with "[GNUPG:] ", and VALIDSIG is the same in this case as GOODSIG, but for fingerprint.

@@ -415,6 +440,13 @@ echo "No tianocore available" && false
%endif
%endif
%if %{with_livepatch}
pushd `pwd`
cd tools/livepatch-build-tools

This comment has been minimized.

@sheep

sheep Jun 16, 2017

Contributor

nit: There is no needs to call pushd $(pwd) first, pushd tools/livepatch-build-tools is equivalent to those two lines.

@sheep

sheep Jun 16, 2017

Contributor

nit: There is no needs to call pushd $(pwd) first, pushd tools/livepatch-build-tools is equivalent to those two lines.

@@ -80,6 +80,10 @@ Source51: xen-kernel.aarch64
Source52: efi-xen.cfg.aarch64
Source53: edk2-bc54e50e0fe03c570014f363b547426913e92449.tar.gz
%if %{with_livepatch}
Source60: livepatch-tools-2af6f1aa62334f8c3d66cf2c5043686833de6cc6.tar.gz

This comment has been minimized.

@sheep

sheep Jun 16, 2017

Contributor

This source name is different than the value of LIVEPATCH_CSET.

@sheep

sheep Jun 16, 2017

Contributor

This source name is different than the value of LIVEPATCH_CSET.

This comment has been minimized.

@gwd

gwd Jun 20, 2017

Member

I'll fix this on check-in; as discussed I think the other things we can fix up later.

@gwd

gwd Jun 20, 2017

Member

I'll fix this on check-in; as discussed I think the other things we can fix up later.

- Bump version due to disabling xsmflash
* Tue Jun 13 2017 Sarah Newman <srn@prgmr.com> 4.8.1-3.el6.centos
- unused build number

This comment has been minimized.

@sheep

sheep Jun 16, 2017

Contributor

nit: I don't think this is needed. If it is unused, then there is no reason to bump the package version number (or called "Release:" in the spec file). It's probably usefull to bump the version only once in this pull request.

@sheep

sheep Jun 16, 2017

Contributor

nit: I don't think this is needed. If it is unused, then there is no reason to bump the package version number (or called "Release:" in the spec file). It's probably usefull to bump the version only once in this pull request.

@gwd

This comment has been minimized.

Show comment
Hide comment
@gwd

gwd Jun 20, 2017

Member

OK, I've made some modifications and pushed the results; I've also built packages for C6 and C7 and tagged them with -testing.

Member

gwd commented Jun 20, 2017

OK, I've made some modifications and pushed the results; I've also built packages for C6 and C7 and tagged them with -testing.

@gwd gwd closed this Jun 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment