[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] Xen Security Advisory 155 (CVE-2015-8550) - paravirtualized drivers incautious about shared memory

On Tue, Dec 22, 2015 at 10:06:25AM -0500, Eric Shelton wrote:
> The XSA mentions that "PV frontend patches will be developed and
> released (publicly) after the embargo date."  Has anything been done
> towards this that should also be incorporated into MiniOS?  On a
> system utilizing a "driver domain," where a backend is running on a
> domain that is considered unprivileged and untrusted (such as the
> example described in http://wiki.xenproject.org/wiki/Driver_Domain),
> it seems XSA-155-style double fetch vulnerabilities in the frontends
> are also a potential security concern, and should be eliminated.
> However, perhaps that does not include pcifront, since pciback would
> always be running in dom0.

And BTW the same applies to Linux frontends, for which also I haven't seen
any public development. In attachment my email to
xen-security-issues-discuss list (sent during embargo), with patches
attached there. I haven't got any response.

PS Dropping minios-devel

Best Regards,
Marek Marczykowski-GÃrecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
--- Begin Message ---
On Fri, Dec 11, 2015 at 11:52:17AM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Dec 11, 2015 at 12:03:01PM +0100, Marek Marczykowski-GÃrecki wrote:
> > Hi,
> > 
> > In advisory it's said that PV frontend patches will be developed and
> > released (publicly) after the embargo date. But in some cases (namely:
> > if driver domains are in use), frontends may be more trusted/privileged
> > than backends. So it would make sense to consider developing frontend
> > patches before embargo end date. At least for the most common ones:
> > network and block.
> > 
> > Do you have some list of places that needs to be fixed? I can try to
> > help with patches, but need some input on this...

Patches (against 4.4) for netfront and blkfront attached. I don't have
test case for actual XSA155, but at least it doesn't break normal usage.
Below are some comments/questions.

Sorry for the delay.

> Any code where the shared page is being accessed. For example
> blkif_interrupt.

Added RING_COPY_RESPONSE here. Probably it would be enough to use
just READ_ONCE(bret->operation), but blkif_response isn't that big, so
safer option is better.

The other places in block frontend are about preparing request - it
should not reuse data already put into shared ring, because (malicious)
backend might already changed it. Generally added local structure on the
stack, filled there and only then put in on the ring. And save a copy to
info->shadow[id].req (the original one, not the one from shared ring!).
Maybe it would be better to use info->shadow[id].req in the first place
(and then copy it to the shared ring)? Theoretically one memory copy
less. Does it matter?

> We probably should add another macro: RING_COPY_RESPONSE which
> would replace the RING_GET_RESPONSE and copy the response to
> an on stack variable and then operated on.
> Ditto with xennet_alloc_rx_buffers and the RING_GET_REQUEST.

I'm not sure about this one - there is only write to that request, no
reads. Or maybe I'm missing some obscure code there?

> And
> xennet_tx_buf_gc.

Additionally added BUG_ON(id >= NET_TX_RING_SIZE). Not sure about the
impact of backend giving out of range id, so make sure it's DoS in the
_worst_ case.

> There is also xennet_tx_setup_grant.

It looks like the tx pointer outlive this function. So instead of
changing it to local copy, made sure no data is read from that structure
(only written).

Similar in handling gso in xennet_start_xmit - it's only filled with

Not sure about xennet_move_rx_slot, but I guess it's safe.

> xen-pcifront looks to be safe - it memcopies the fields. However
> as I've found out - doing 'memcpy' does not mean the compiler
> will do that - it may convert that just to a pointer operation.
> So an barrier() will have to be sprinkled in 'do_pci_op'.

There is already wmb() after the first memcpy. Is it necessary to have
another one before the memcpy in the other direction?

$ shasum -a 256 *.patch

Best Regards,
Marek Marczykowski-GÃrecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: xsa155-linux-0008-xen-Add-RING_COPY_RESPONSE.patch
Description: Text document

Attachment: xsa155-linux-0009-xen-netfront-copy-response-out-of-shared-buffer-befo.patch
Description: Text document

Attachment: xsa155-linux-0010-xen-netfront-do-not-use-data-already-exposed-to-back.patch
Description: Text document

Attachment: xsa155-linux-0011-xen-netfront-add-range-check-for-Tx-response-id.patch
Description: Text document

Attachment: xsa155-linux-0012-xen-blkfront-make-local-copy-of-response-before-usin.patch
Description: Text document

Attachment: xsa155-linux-0013-xen-blkfront-prepare-request-locally-only-then-put-i.patch
Description: Text document

Attachment: pgpvuq3A0QpJR.pgp
Description: PGP signature

Xen-security-issues-discuss mailing list

--- End Message ---

Attachment: pgprgNVx88raX.pgp
Description: PGP signature

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.