Re: [Qemu-devel] Design Decision for KVM based anti rootkit
On 16/06/18 12:49, Ahmed Soliman wrote: > > To wrap things up, the basic design will be a method for communication > between host and guest is guest can request certain pages to be read > only, and then host will force them to be read-only by guest until > next guest reboot, then it will impossible for guest OS to have them > as RW again. The choice of which pages to be set as read only is the > guest's. So this way mixed pages can still be mixed with R/W content > even if holds kernel code. It's not clear how this increases security. What threats is this protecting again? As an attacker, modifying the sensitive pages (kernel text?) will require either: a) altering the existing mappings for these (to make them read-write or user-writable for example); or b) creating aliased mappings with suitable permissions. If the attacker can modify page tables in this way then it can also bypass the suggested hypervisor's read-only protection by changing the mappings to point to a unprotected page. David
[Qemu-devel] [PATCH 2/2] vhost-user-scsi: reset the device if supported
If the vhost-user-scsi backend supports the VHOST_USER_F_RESET_DEVICE
protocol feature, then the device can be reset when requested.
If this feature is not supported, do not try a reset as this will send
a VHOST_USER_RESET_OWNER that the backend is not expecting,
potentially putting into an inoperable state.
Signed-off-by: David Vrabel
---
hw/scsi/vhost-user-scsi.c | 24
1 file changed, 24 insertions(+)
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 9389ed48e0..15e2dabebb 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -37,6 +37,10 @@ static const int user_feature_bits[] = {
VHOST_INVALID_FEATURE_BIT
};
+enum VhostUserProtocolFeature {
+VHOST_USER_PROTOCOL_F_RESET_DEVICE = 8,
+};
+
static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
{
VHostUserSCSI *s = (VHostUserSCSI *)vdev;
@@ -60,6 +64,25 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev,
uint8_t status)
}
}
+static void vhost_user_scsi_reset(VirtIODevice *vdev)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(vdev);
+struct vhost_dev *dev = &vsc->dev;
+
+/*
+ * Historically, reset was not implemented so only reset devices
+ * that are expecting it.
+ */
+if (!virtio_has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_RESET_DEVICE)) {
+return;
+}
+
+if (dev->vhost_ops->vhost_reset_device) {
+dev->vhost_ops->vhost_reset_device(dev);
+}
+}
+
static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
{
}
@@ -172,6 +195,7 @@ static void vhost_user_scsi_class_init(ObjectClass *klass,
void *data)
vdc->get_features = vhost_user_scsi_get_features;
vdc->set_config = vhost_scsi_common_set_config;
vdc->set_status = vhost_user_scsi_set_status;
+vdc->reset = vhost_user_scsi_reset;
fwc->get_dev_path = vhost_scsi_common_get_fw_dev_path;
}
--
2.11.0
[Qemu-devel] [PATCH 1/2] vhost-user: add VHOST_USER_RESET_DEVICE to reset devices
Add a VHOST_USER_RESET_DEVICE message which will reset the vhost user
backend. Disabling all rings, and resetting all internal state, ready
for the backend to be reinitialized.
A backend has to report it supports this features with the
VHOST_USER_PROTOCOL_F_RESET_DEVICE protocol feature bit. If it does
so, the new message is used instead of sending a RESET_OWNER which has
had inconsistent implementations.
Signed-off-by: David Vrabel
---
docs/interop/vhost-user.txt | 16
hw/virtio/vhost-user.c | 8 +++-
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
index cb3a7595aa..c5faa9fff8 100644
--- a/docs/interop/vhost-user.txt
+++ b/docs/interop/vhost-user.txt
@@ -369,6 +369,7 @@ Protocol features
#define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5
#define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6
#define VHOST_USER_PROTOCOL_F_CRYPTO_SESSION 7
+#define VHOST_USER_PROTOCOL_F_RESET_DEVICE 8
Master message types
@@ -689,6 +690,21 @@ Master message types
feature has been successfully negotiated.
It's a required feature for crypto devices.
+ * VHOST_USER_RESET_DEVICE
+
+ Id: 28
+ Equivalent ioctl: N/A
+ Master payload: N/A
+ Slave payload: N/A
+
+ Ask the vhost user backend to disable all rings and reset all
+ internal device state to the initial state, ready to be
+ reinitialized. The backend retains ownership of the device
+ throughout the reset operation.
+
+ Only valid if the VHOST_USER_PROTOCOL_F_RESET_DEVICE protocol
+ feature is set by the backend.
+
Slave message types
---
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 41ff5cff41..67207bfe8a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -41,6 +41,7 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_SLAVE_REQ = 5,
VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
+VHOST_USER_PROTOCOL_F_RESET_DEVICE = 8,
VHOST_USER_PROTOCOL_F_MAX
};
@@ -76,6 +77,7 @@ typedef enum VhostUserRequest {
VHOST_USER_SET_CONFIG = 25,
VHOST_USER_CREATE_CRYPTO_SESSION = 26,
VHOST_USER_CLOSE_CRYPTO_SESSION = 27,
+VHOST_USER_RESET_DEVICE = 28,
VHOST_USER_MAX
} VhostUserRequest;
@@ -641,10 +643,14 @@ static int vhost_user_set_owner(struct vhost_dev *dev)
static int vhost_user_reset_device(struct vhost_dev *dev)
{
VhostUserMsg msg = {
-.hdr.request = VHOST_USER_RESET_OWNER,
.hdr.flags = VHOST_USER_VERSION,
};
+msg.hdr.request = virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_RESET_DEVICE)
+? VHOST_USER_RESET_DEVICE
+: VHOST_USER_RESET_OWNER;
+
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
return -1;
}
--
2.11.0
[Qemu-devel] [PATCH 0/2] vhost-user-scsi: add message for device reset
When a virtio scsi device is reset by a guest, the reset is not passed on to the vhost-user backend. This reset may be necessary if a device driver is restarted or the device is handed off between (for example) SeaBIOS and the OS. Iff the vhost-user-scsi backend reports that it supports a new "reset device" feature, QEMU will send a VHOST_USER_RESET_DEVICE message when the guest resets the virtio device. Existing backends are unaffected. David
Re: [Qemu-devel] [Xen-devel] [PATCH v4 1/2] Interface for grant copy operation in libs.
On 03/08/16 15:36, David Vrabel wrote: > On 02/08/16 15:06, Paulina Szubarczyk wrote: >> >> +/** >> + * Copy memory from or to grant references. The information of each >> operations >> + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value >> indicate >> + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref). >> + * >> + * The sum of fields @offset[i] and @len[i] of >> 'xengnttab_grant_copy_segment_t' >> + * should not exceed XEN_PAGE_SIZE > > "For each segment, @virt may cross a page boundary but @offset + @len > must be less than XEN_PAGE_SIZE." might be better. This should be: "For each segment, @virt may cross a page boundary but @offset + @len must not exceed XEN_PAGE_SIZE." David
Re: [Qemu-devel] [Xen-devel] [PATCH v4 1/2] Interface for grant copy operation in libs.
On 02/08/16 15:06, Paulina Szubarczyk wrote: > > +/** > + * Copy memory from or to grant references. The information of each > operations > + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value > indicate > + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref). > + * > + * The sum of fields @offset[i] and @len[i] of > 'xengnttab_grant_copy_segment_t' > + * should not exceed XEN_PAGE_SIZE "For each segment, @virt may cross a page boundary but @offset + @len must be less than XEN_PAGE_SIZE." might be better. With or without the above change: Reviewed-by: David Vrabel David
Re: [Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)
On 21/06/16 08:35, Paolo Bonzini wrote: > > > On 21/06/2016 03:44, Jason Wang wrote: >> >> >> On 2016年06月21日 01:53, David Vrabel wrote: >>> Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port >>> TallyCounters to vmstate) introduced in incompatibility in the v4 >>> format as it omitted the RxOkMul counter. >>> >>> There are presumably no users that were impacted by the v4 to v4' >>> breakage, so increase the save version to 5 and re-add the field, >>> keeping backward compatibility with v4'. >>> >>> We can't have a field conditional on the section version in >>> vmstate_tally_counters since this version checked would not be the >>> section version (but the version defined in this structure). So, move >>> all the fields into the main state structure. >>> >>> Signed-off-by: David Vrabel >> >> Migration to old version is important for the user and this patch seems >> to break this. How about something like: >> >> - introduce a subsection for RXOKMul >> - only migrate it for new version (e.g >= 2.7) I don't see how this can work with snapshots where the QEMU version that is going to restore the snapshot is not known in advance. > Introducing a subsection is not really necessary if the value is going > to be migrated always, and upstream generally does not have "migrate it > only in some version" checks. This is left for downstreams to implement > if they care. We just don't have the manpower to ensure that migration > to older versions works between all releases of QEMU. So is this patch acceptable as is? David
[Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)
Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
TallyCounters to vmstate) introduced in incompatibility in the v4
format as it omitted the RxOkMul counter.
There are presumably no users that were impacted by the v4 to v4'
breakage, so increase the save version to 5 and re-add the field,
keeping backward compatibility with v4'.
We can't have a field conditional on the section version in
vmstate_tally_counters since this version checked would not be the
section version (but the version defined in this structure). So, move
all the fields into the main state structure.
Signed-off-by: David Vrabel
---
Cc: Jason Wang
---
hw/net/rtl8139.c | 40 ++--
1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 562c1fd..8ccd1d3 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1352,29 +1352,6 @@ static void RTL8139TallyCounters_dma_write(RTL8139State
*s, dma_addr_t tc_addr)
pci_dma_write(d, tc_addr + 62,(uint8_t *)&val16, 2);
}
-/* Loads values of tally counters from VM state file */
-
-static const VMStateDescription vmstate_tally_counters = {
-.name = "tally_counters",
-.version_id = 1,
-.minimum_version_id = 1,
-.fields = (VMStateField[]) {
-VMSTATE_UINT64(TxOk, RTL8139TallyCounters),
-VMSTATE_UINT64(RxOk, RTL8139TallyCounters),
-VMSTATE_UINT64(TxERR, RTL8139TallyCounters),
-VMSTATE_UINT32(RxERR, RTL8139TallyCounters),
-VMSTATE_UINT16(MissPkt, RTL8139TallyCounters),
-VMSTATE_UINT16(FAE, RTL8139TallyCounters),
-VMSTATE_UINT32(Tx1Col, RTL8139TallyCounters),
-VMSTATE_UINT32(TxMCol, RTL8139TallyCounters),
-VMSTATE_UINT64(RxOkPhy, RTL8139TallyCounters),
-VMSTATE_UINT64(RxOkBrd, RTL8139TallyCounters),
-VMSTATE_UINT16(TxAbt, RTL8139TallyCounters),
-VMSTATE_UINT16(TxUndrn, RTL8139TallyCounters),
-VMSTATE_END_OF_LIST()
-}
-};
-
static void rtl8139_ChipCmd_write(RTL8139State *s, uint32_t val)
{
DeviceState *d = DEVICE(s);
@@ -3222,7 +3199,7 @@ static void rtl8139_pre_save(void *opaque)
static const VMStateDescription vmstate_rtl8139 = {
.name = "rtl8139",
-.version_id = 4,
+.version_id = 5,
.minimum_version_id = 3,
.post_load = rtl8139_post_load,
.pre_save = rtl8139_pre_save,
@@ -3293,8 +3270,19 @@ static const VMStateDescription vmstate_rtl8139 = {
VMSTATE_UINT32(TimerInt, RTL8139State),
VMSTATE_INT64(TCTR_base, RTL8139State),
-VMSTATE_STRUCT(tally_counters, RTL8139State, 0,
- vmstate_tally_counters, RTL8139TallyCounters),
+VMSTATE_UINT64(tally_counters.TxOk, RTL8139State),
+VMSTATE_UINT64(tally_counters.RxOk, RTL8139State),
+VMSTATE_UINT64(tally_counters.TxERR, RTL8139State),
+VMSTATE_UINT32(tally_counters.RxERR, RTL8139State),
+VMSTATE_UINT16(tally_counters.MissPkt, RTL8139State),
+VMSTATE_UINT16(tally_counters.FAE, RTL8139State),
+VMSTATE_UINT32(tally_counters.Tx1Col, RTL8139State),
+VMSTATE_UINT32(tally_counters.TxMCol, RTL8139State),
+VMSTATE_UINT64(tally_counters.RxOkPhy, RTL8139State),
+VMSTATE_UINT64(tally_counters.RxOkBrd, RTL8139State),
+VMSTATE_UINT32_V(tally_counters.RxOkMul, RTL8139State, 5),
+VMSTATE_UINT16(tally_counters.TxAbt, RTL8139State),
+VMSTATE_UINT16(tally_counters.TxUndrn, RTL8139State),
VMSTATE_UINT32_V(cplus_enabled, RTL8139State, 4),
VMSTATE_END_OF_LIST()
--
2.1.4
[Qemu-devel] [PATCHv1] rtl8139: save/load RxMulOk counter (again)
Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
TallyCounters to vmstate) introduced in incompatibility in the v4
format as it omitted the RxOkMul counter.
There are presumably no users that were impacted by the v4 to v4'
breakage, so increase the save version to 5 and re-add the field,
keeping backward compatibility with v4'.
Signed-off-by: David Vrabel
---
hw/net/rtl8139.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 562c1fd..243dcd4 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1369,6 +1369,7 @@ static const VMStateDescription vmstate_tally_counters = {
VMSTATE_UINT32(TxMCol, RTL8139TallyCounters),
VMSTATE_UINT64(RxOkPhy, RTL8139TallyCounters),
VMSTATE_UINT64(RxOkBrd, RTL8139TallyCounters),
+VMSTATE_UINT32_V(RxOkMul, RTL8139TallyCounters, 5),
VMSTATE_UINT16(TxAbt, RTL8139TallyCounters),
VMSTATE_UINT16(TxUndrn, RTL8139TallyCounters),
VMSTATE_END_OF_LIST()
@@ -3222,7 +3223,7 @@ static void rtl8139_pre_save(void *opaque)
static const VMStateDescription vmstate_rtl8139 = {
.name = "rtl8139",
-.version_id = 4,
+.version_id = 5,
.minimum_version_id = 3,
.post_load = rtl8139_post_load,
.pre_save = rtl8139_pre_save,
--
2.1.4
Re: [Qemu-devel] [Xen-devel] [PATCH v2 2/3] xenfb: move xen_rmb to the correct location
On 12/04/16 11:43, Wei Liu wrote:
> It should be placed before first time producer and consumer are used.
This change isn't necessary and is confusing as this is not what this
barrier is for.
The barrier needs to be between the load of prod and the load of the
ring contents (there's even a comment that says this). This pairs with
the corresponding write barrier between the store of the ring contents
and the store of prod (in the other end).
David
>
> Signed-off-by: Wei Liu
> ---
> Cc: Stefano Stabellini
> Cc: Anthony Perard
>
> Backport candidate to our own tree.
> ---
> hw/display/xenfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 9866dfd..7f4fad7 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -775,10 +775,10 @@ static void xenfb_handle_events(struct XenFB *xenfb)
>
> prod = page->out_prod;
> out_cons = page->out_cons;
> +xen_rmb();
> if (prod - out_cons > XENFB_OUT_RING_LEN) {
> return;
> }
> -xen_rmb(); /* ensure we see ring contents up to prod */
> for (cons = out_cons; cons != prod; cons++) {
> union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
> uint8_t type = event->type;
>
Re: [Qemu-devel] [Xen-devel] [PATCH] xen: write information about supported backends
On 30/03/16 15:10, Juergen Gross wrote: > Add a Xenstore directory for each supported pv backend. This will allow > Xen tools to decide which backend type to use in case there are > multiple possibilities. > > The information is added under > /local/domain//device-model//backends > before the "running" state is written to Xenstore. Using a directory > for each directory enables us to add parameters for specific backends > in the future. "Using a directory for each backend..."? > > In order to reuse the Xenstore directory creation already present in > hw/xen/xen_devconfig.c move the related functions to > hw/xen/xen_backend.c where they fit better. Why Xenstore and not QMP? David
Re: [Qemu-devel] [Xen-devel] [PATCH] xenfb.c: avoid expensive loops when prod <= out_cons
On 06/01/16 12:08, Stefano Stabellini wrote: > If the frontend sets out_cons to a value higher than out_prod, it will > cause xenfb_handle_events to loop about 2^32 times. Avoid that by using > better checks at the beginning of the function. You can't use less than to compare prod and cons because they wrap. You need to compare (prod - cons) against ring size (or similar) to check for overflow. See RING_REQUEST_PROD_OVERFLOW() etc. David
Re: [Qemu-devel] [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
On 20/12/15 09:25, Michael S. Tsirkin wrote: > > I noticed that drivers/xen/xenbus/xenbus_comms.c uses > full memory barriers to communicate with the other side. > For example: > > /* Must write data /after/ reading the consumer index. * */ > mb(); > > memcpy(dst, data, avail); > data += avail; > len -= avail; > > /* Other side must not see new producer until data is * > there. */ > wmb(); > intf->req_prod += avail; > > /* Implies mb(): other side will see the updated producer. */ > notify_remote_via_evtchn(xen_store_evtchn); > > To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb > would be sufficient, so mb() and wmb() here are only needed if > a non-SMP guest runs on an SMP host. > > Is my analysis correct? For x86, yes. For arm/arm64 I think so, but would prefer one of the Xen arm maintainers to confirm. In particular, whether inner-shareable barriers are sufficient for memory shared with the hypervisor. > So what I'm suggesting is something like the below patch, > except instead of using virtio directly, a new set of barriers > that behaves identically for SMP and non-SMP guests will be introduced. > > And of course the weak barriers flag is not needed for Xen - > that's a virtio only thing. > > For example: > > smp_pv_wmb() > smp_pv_rmb() > smp_pv_mb() The smp_ prefix doesn't make a lot of sense to me here since these barriers are going to be the same whether the kernel is SMP or not. David
Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xenfb: Add comment documentation
On 22/09/14 10:04, Owen smith wrote: > Add documentation for page-ref, page-gref and event-channel. > > Signed-off-by: Owen smith > --- > xen/include/public/io/fbif.h | 25 + > 1 file changed, 25 insertions(+) > > diff --git a/xen/include/public/io/fbif.h b/xen/include/public/io/fbif.h > index cc25aab..ba3f524 100644 > --- a/xen/include/public/io/fbif.h > +++ b/xen/include/public/io/fbif.h > @@ -26,6 +26,31 @@ > #ifndef __XEN_PUBLIC_IO_FBIF_H__ > #define __XEN_PUBLIC_IO_FBIF_H__ > > +/* > + * Frontend XenStore Nodes > + * --- > + * > + * page-ref > + * Values: > + * Optional, "page-gref" is used if "page-ref" is not set. > + * > + * The MFN of a page of memory for the shared ring structures. If not > + * present, "page-gref" must be set.page-ref" overrides "page-gref". I think you mean GFN here, not MFN. > + * page-gref > + * Values: > + * Only required if "page-ref" is NOT set. > + * > + * A grant reference to the memory page to be mapped for the shared ring > + * structures. Must be present if "page-ref" is not present. Should there be some negotiation for which of page-ref or page-gref is required/preferred by the backend? The same applies to kbdif as well. David
