| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d5d320fc-036e-0fc5-ed91-90cbf9b9c17f@redhat.com>
Date: Thu, 7 May 2020 14:43:03 +0200
From: Auger Eric <eric.auger@...hat.com>
To: Bharat Bhushan <bbhushan2@...vell.com>,
"Michael S. Tsirkin" <mst@...hat.com>
Cc: "jean-philippe@...aro.org" <jean-philippe@...aro.org>,
"joro@...tes.org" <joro@...tes.org>,
"jasowang@...hat.com" <jasowang@...hat.com>,
"virtualization@...ts.linux-foundation.org"
<virtualization@...ts.linux-foundation.org>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"eric.auger.pro@...il.com" <eric.auger.pro@...il.com>
Subject: Re: [EXT] Re: [PATCH v5] iommu/virtio: Use page size bitmap supported
by endpoint
Hi Bharat,
On 5/7/20 1:24 PM, Bharat Bhushan wrote:
>
>
>> -----Original Message-----
>> From: Michael S. Tsirkin <mst@...hat.com>
>> Sent: Wednesday, May 6, 2020 5:53 AM
>> To: Bharat Bhushan <bbhushan2@...vell.com>
>> Cc: jean-philippe@...aro.org; joro@...tes.org; jasowang@...hat.com;
>> virtualization@...ts.linux-foundation.org; iommu@...ts.linux-foundation.org;
>> linux-kernel@...r.kernel.org; eric.auger.pro@...il.com; eric.auger@...hat.com
>> Subject: [EXT] Re: [PATCH v5] iommu/virtio: Use page size bitmap supported by
>> endpoint
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On Tue, May 05, 2020 at 03:00:04PM +0530, Bharat Bhushan wrote:
>>> Different endpoint can support different page size, probe endpoint if
>>> it supports specific page size otherwise use global page sizes.
>>>
>>> Signed-off-by: Bharat Bhushan <bbhushan2@...vell.com>
>>> ---
>>> v4->v5:
>>> - Rebase to Linux v5.7-rc4
>>>
>>> v3->v4:
>>> - Fix whitespace error
>>>
>>> v2->v3:
>>> - Fixed error return for incompatible endpoint
>>> - __u64 changed to __le64 in header file
>>>
>>> drivers/iommu/virtio-iommu.c | 48 ++++++++++++++++++++++++++++---
>>> include/uapi/linux/virtio_iommu.h | 7 +++++
>>> 2 files changed, 51 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/virtio-iommu.c
>>> b/drivers/iommu/virtio-iommu.c index d5cac4f46ca5..9513d2ab819e 100644
>>> --- a/drivers/iommu/virtio-iommu.c
>>> +++ b/drivers/iommu/virtio-iommu.c
>>> @@ -78,6 +78,7 @@ struct viommu_endpoint {
>>> struct viommu_dev *viommu;
>>> struct viommu_domain *vdomain;
>>> struct list_head resv_regions;
>>> + u64 pgsize_bitmap;
>>> };
>>>
>>> struct viommu_request {
>>> @@ -415,6 +416,19 @@ static int viommu_replay_mappings(struct
>> viommu_domain *vdomain)
>>> return ret;
>>> }
>>>
>>> +static int viommu_set_pgsize_bitmap(struct viommu_endpoint *vdev,
>>> + struct virtio_iommu_probe_pgsize_mask *mask,
>>> + size_t len)
>>> +{
>>> + u64 pgsize_bitmap = le64_to_cpu(mask->pgsize_bitmap);
>>> +
>>> + if (len < sizeof(*mask))
>>
>> This is too late to validate length, you have dereferenced it already.
>> do it before the read pls.
>
> Yes, Will change here and other places as well
>
>>
>>> + return -EINVAL;
>>
>> OK but note that guest will then just proceed to ignore the property. Is that really
>> OK? Wouldn't host want to know?
>
>
> Guest need to be in sync with device, so yes seems like guest need to tell device which page-size-mask it is using.
>
> Corresponding spec change patch (https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg06214.html)
>
> Would like Jean/Eric to comment here as well.
why can't we fail the probe request in that case? This is a misbehaving
device that reports malformed property, right?
Thanks
Eric
>
>>
>>
>>> +
>>> + vdev->pgsize_bitmap = pgsize_bitmap;
>>
>> what if bitmap is 0? Is that a valid size? I see a bunch of BUG_ON with that value ...
>
> As per spec proposed device is supposed to set at-least one bit.
> Will add a bug_on her.
> Should we add bug_on or switch to global config page-size mask if this is zero (notify device which page-size-mask it is using).
>
>>
>> I also see a bunch of code like e.g. this:
>>
>> pg_size = 1UL << __ffs(pgsize_bitmap);
>>
>> which probably won't DTRT on a 32 bit guest if the bitmap has bits set in the high
>> word.
>>
>
> My thought is that in that case viommu_domain_finalise() will fail, do not proceed.
>
>>
>>
>>> + return 0;
>>> +}
>>> +
>>> static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
>>> struct virtio_iommu_probe_resv_mem *mem,
>>> size_t len)
>>> @@ -499,6 +513,9 @@ static int viommu_probe_endpoint(struct viommu_dev
>> *viommu, struct device *dev)
>>> case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
>>> ret = viommu_add_resv_mem(vdev, (void *)prop, len);
>>> break;
>>> + case VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK:
>>> + ret = viommu_set_pgsize_bitmap(vdev, (void *)prop, len);
>>> + break;
>>> default:
>>> dev_err(dev, "unknown viommu prop 0x%x\n", type);
>>> }
>>> @@ -630,7 +647,7 @@ static int viommu_domain_finalise(struct
>>> viommu_endpoint *vdev,
>>>
>>> vdomain->id = (unsigned int)ret;
>>>
>>> - domain->pgsize_bitmap = viommu->pgsize_bitmap;
>>> + domain->pgsize_bitmap = vdev->pgsize_bitmap;
>>> domain->geometry = viommu->geometry;
>>>
>>> vdomain->map_flags = viommu->map_flags;
>>> @@ -654,6 +671,29 @@ static void viommu_domain_free(struct iommu_domain
>> *domain)
>>> kfree(vdomain);
>>> }
>>>
>>> +/*
>>> + * Check whether the endpoint's capabilities are compatible with
>>> +other
>>> + * endpoints in the domain. Report any inconsistency.
>>> + */
>>> +static bool viommu_endpoint_is_compatible(struct viommu_endpoint *vdev,
>>> + struct viommu_domain *vdomain) {
>>> + struct device *dev = vdev->dev;
>>> +
>>> + if (vdomain->viommu != vdev->viommu) {
>>> + dev_err(dev, "cannot attach to foreign vIOMMU\n");
>>> + return false;
>>> + }
>>> +
>>> + if (vdomain->domain.pgsize_bitmap != vdev->pgsize_bitmap) {
>>> + dev_err(dev, "incompatible domain bitmap 0x%lx != 0x%llx\n",
>>> + vdomain->domain.pgsize_bitmap, vdev->pgsize_bitmap);
>>> + return false;
>>> + }
>>
>> I'm confused by this. So let's assume host supports pages sizes of 4k, 2M, 1G. It
>> signals this in the properties. Nice.
>> Now domain supports 4k, 2M and that's all. Why is that a problem?
>> Just don't use 1G ...
>
> Is not it too to change the existing domain properties, for devices already attached to domain? New devices must match to domain page-size.
>
>>
>>
>>> +
>>> + return true;
>>> +}
>>> +
>>> static int viommu_attach_dev(struct iommu_domain *domain, struct
>>> device *dev) {
>>> int i;
>>> @@ -670,9 +710,8 @@ static int viommu_attach_dev(struct iommu_domain
>> *domain, struct device *dev)
>>> * owns it.
>>> */
>>> ret = viommu_domain_finalise(vdev, domain);
>>> - } else if (vdomain->viommu != vdev->viommu) {
>>> - dev_err(dev, "cannot attach to foreign vIOMMU\n");
>>> - ret = -EXDEV;
>>> + } else if (!viommu_endpoint_is_compatible(vdev, vdomain)) {
>>> + ret = -EINVAL;
>>> }
>>> mutex_unlock(&vdomain->mutex);
>>>
>>> @@ -886,6 +925,7 @@ static int viommu_add_device(struct device *dev)
>>>
>>> vdev->dev = dev;
>>> vdev->viommu = viommu;
>>> + vdev->pgsize_bitmap = viommu->pgsize_bitmap;
>>> INIT_LIST_HEAD(&vdev->resv_regions);
>>> dev_iommu_priv_set(dev, vdev);
>>>
>>> diff --git a/include/uapi/linux/virtio_iommu.h
>>> b/include/uapi/linux/virtio_iommu.h
>>> index 48e3c29223b5..2cced7accc99 100644
>>> --- a/include/uapi/linux/virtio_iommu.h
>>> +++ b/include/uapi/linux/virtio_iommu.h
>>
>> As any virtio UAPI change, you need to copy virtio TC at some point before this is
>> merged ...
>
> Jean already send patch for same
> https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg06214.html
>
> Do we need to do anything additional?
>
>>
>>> @@ -111,6 +111,7 @@ struct virtio_iommu_req_unmap {
>>>
>>> #define VIRTIO_IOMMU_PROBE_T_NONE 0
>>> #define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
>>> +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK 2
>>>
>>> #define VIRTIO_IOMMU_PROBE_T_MASK 0xfff
>>>
>>
>> Does host need to know that guest will ignore the page size mask?
>> Maybe we need a feature bit.
>>
>>> @@ -119,6 +120,12 @@ struct virtio_iommu_probe_property {
>>> __le16 length;
>>> };
>>>
>>> +struct virtio_iommu_probe_pgsize_mask {
>>> + struct virtio_iommu_probe_property head;
>>> + __u8 reserved[4];
>>> + __le64 pgsize_bitmap;
>>> +};
>>> +
>>
>> This is UAPI. Document the format of pgsize_bitmap please.
>
> Ok,
>
> Thanks
> -Bharat
>
>>
>>
>>> #define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0
>>> #define VIRTIO_IOMMU_RESV_MEM_T_MSI 1
>>>
>>> --
>>> 2.17.1
>
Powered by blists - more mailing lists