[v5,3/3] vfio/pci: Decouple PCI_COMMAND_MEMORY bit checks from is_virtfn

Commit Message

Matthew Rosato Sept. 10, 2020, 2:59 p.m. UTC

While it is true that devices with is_virtfn=1 will have a Memory Space
Enable bit that is hard-wired to 0, this is not the only case where we
see this behavior -- For example some bare-metal hypervisors lack
Memory Space Enable bit emulation for devices not setting is_virtfn
(s390). Fix this by instead checking for the newly-added
no_command_memory bit which directly denotes the need for
PCI_COMMAND_MEMORY emulation in vfio.

Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
Signed-off-by: Matthew Rosato <[e-mail]>
Reviewed-by: Niklas Schnelle <[e-mail]>
Reviewed-by: Pierre Morel <[e-mail]>
---
 drivers/vfio/pci/vfio_pci_config.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

Comments

Matthew Rosato Sept. 21, 2020, 12:43 p.m. UTC | #1

On 9/10/20 10:59 AM, Matthew Rosato wrote:
> While it is true that devices with is_virtfn=1 will have a Memory Space
> Enable bit that is hard-wired to 0, this is not the only case where we
> see this behavior -- For example some bare-metal hypervisors lack
> Memory Space Enable bit emulation for devices not setting is_virtfn
> (s390). Fix this by instead checking for the newly-added
> no_command_memory bit which directly denotes the need for
> PCI_COMMAND_MEMORY emulation in vfio.
> 
> Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
> Signed-off-by: Matthew Rosato <[e-mail]>
> Reviewed-by: Niklas Schnelle <[e-mail]>
> Reviewed-by: Pierre Morel <[e-mail]>

Polite ping on this patch as the other 2 have now received maintainer 
ACKs or reviews.  I'm concerned about this popping up in distros as 
abafbc551fdd was a CVE fix.  Related, see question from the cover:

- Restored the fixes tag to patch 3 (but the other 2 patches are
   now pre-reqs -- cc stable 5.8?)

Thanks,
Matt

> ---
>   drivers/vfio/pci/vfio_pci_config.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index d98843f..5076d01 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -406,7 +406,7 @@ bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
>   	 * PF SR-IOV capability, there's therefore no need to trigger
>   	 * faults based on the virtual value.
>   	 */
> -	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);
> +	return pdev->no_command_memory || (cmd & PCI_COMMAND_MEMORY);
>   }
>   
>   /*
> @@ -520,8 +520,8 @@ static int vfio_basic_config_read(struct vfio_pci_device *vdev, int pos,
>   
>   	count = vfio_default_config_read(vdev, pos, count, perm, offset, val);
>   
> -	/* Mask in virtual memory enable for SR-IOV devices */
> -	if (offset == PCI_COMMAND && vdev->pdev->is_virtfn) {
> +	/* Mask in virtual memory enable */
> +	if (offset == PCI_COMMAND && vdev->pdev->no_command_memory) {
>   		u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
>   		u32 tmp_val = le32_to_cpu(*val);
>   
> @@ -589,9 +589,11 @@ static int vfio_basic_config_write(struct vfio_pci_device *vdev, int pos,
>   		 * shows it disabled (phys_mem/io, then the device has
>   		 * undergone some kind of backdoor reset and needs to be
>   		 * restored before we allow it to enable the bars.
> -		 * SR-IOV devices will trigger this, but we catch them later
> +		 * SR-IOV devices will trigger this - for mem enable let's
> +		 * catch this now and for io enable it will be caught later
>   		 */
> -		if ((new_mem && virt_mem && !phys_mem) ||
> +		if ((new_mem && virt_mem && !phys_mem &&
> +		     !pdev->no_command_memory) ||
>   		    (new_io && virt_io && !phys_io) ||
>   		    vfio_need_bar_restore(vdev))
>   			vfio_bar_restore(vdev);
> @@ -1734,12 +1736,14 @@ int vfio_config_init(struct vfio_pci_device *vdev)
>   				 vconfig[PCI_INTERRUPT_PIN]);
>   
>   		vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
> -
> +	}
> +	if (pdev->no_command_memory) {
>   		/*
> -		 * VFs do no implement the memory enable bit of the COMMAND
> -		 * register therefore we'll not have it set in our initial
> -		 * copy of config space after pci_enable_device().  For
> -		 * consistency with PFs, set the virtual enable bit here.
> +		 * VFs and devices that set pdev->no_command_memory do not
> +		 * implement the memory enable bit of the COMMAND register
> +		 * therefore we'll not have it set in our initial copy of
> +		 * config space after pci_enable_device().  For consistency
> +		 * with PFs, set the virtual enable bit here.
>   		 */
>   		*(__le16 *)&vconfig[PCI_COMMAND] |=
>   					cpu_to_le16(PCI_COMMAND_MEMORY);
>

Alex Williamson Sept. 22, 2020, 4:40 p.m. UTC | #2

On Mon, 21 Sep 2020 08:43:29 -0400
Matthew Rosato <[e-mail]> wrote:

> On 9/10/20 10:59 AM, Matthew Rosato wrote:
> > While it is true that devices with is_virtfn=1 will have a Memory Space
> > Enable bit that is hard-wired to 0, this is not the only case where we
> > see this behavior -- For example some bare-metal hypervisors lack
> > Memory Space Enable bit emulation for devices not setting is_virtfn
> > (s390). Fix this by instead checking for the newly-added
> > no_command_memory bit which directly denotes the need for
> > PCI_COMMAND_MEMORY emulation in vfio.
> > 
> > Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
> > Signed-off-by: Matthew Rosato <[e-mail]>
> > Reviewed-by: Niklas Schnelle <[e-mail]>
> > Reviewed-by: Pierre Morel <[e-mail]>  
> 
> Polite ping on this patch as the other 2 have now received maintainer 
> ACKs or reviews.  I'm concerned about this popping up in distros as 
> abafbc551fdd was a CVE fix.  Related, see question from the cover:
> 
> - Restored the fixes tag to patch 3 (but the other 2 patches are
>    now pre-reqs -- cc stable 5.8?)

I've got these queued in my local branch which I'll push to next for
v5.10.  I'm thinking that perhaps the right thing would be to add the
fixes tag to all three patches, otherwise I could see that the PCI/VF
change might get picked as a dependency, but not the s390 specific one.
Does this sound correct to everyone?  Thanks,

Alex

> > ---
> >   drivers/vfio/pci/vfio_pci_config.c | 24 ++++++++++++++----------
> >   1 file changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> > index d98843f..5076d01 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -406,7 +406,7 @@ bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
> >   	 * PF SR-IOV capability, there's therefore no need to trigger
> >   	 * faults based on the virtual value.
> >   	 */
> > -	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);
> > +	return pdev->no_command_memory || (cmd & PCI_COMMAND_MEMORY);
> >   }
> >   
> >   /*
> > @@ -520,8 +520,8 @@ static int vfio_basic_config_read(struct vfio_pci_device *vdev, int pos,
> >   
> >   	count = vfio_default_config_read(vdev, pos, count, perm, offset, val);
> >   
> > -	/* Mask in virtual memory enable for SR-IOV devices */
> > -	if (offset == PCI_COMMAND && vdev->pdev->is_virtfn) {
> > +	/* Mask in virtual memory enable */
> > +	if (offset == PCI_COMMAND && vdev->pdev->no_command_memory) {
> >   		u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
> >   		u32 tmp_val = le32_to_cpu(*val);
> >   
> > @@ -589,9 +589,11 @@ static int vfio_basic_config_write(struct vfio_pci_device *vdev, int pos,
> >   		 * shows it disabled (phys_mem/io, then the device has
> >   		 * undergone some kind of backdoor reset and needs to be
> >   		 * restored before we allow it to enable the bars.
> > -		 * SR-IOV devices will trigger this, but we catch them later
> > +		 * SR-IOV devices will trigger this - for mem enable let's
> > +		 * catch this now and for io enable it will be caught later
> >   		 */
> > -		if ((new_mem && virt_mem && !phys_mem) ||
> > +		if ((new_mem && virt_mem && !phys_mem &&
> > +		     !pdev->no_command_memory) ||
> >   		    (new_io && virt_io && !phys_io) ||
> >   		    vfio_need_bar_restore(vdev))
> >   			vfio_bar_restore(vdev);
> > @@ -1734,12 +1736,14 @@ int vfio_config_init(struct vfio_pci_device *vdev)
> >   				 vconfig[PCI_INTERRUPT_PIN]);
> >   
> >   		vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
> > -
> > +	}
> > +	if (pdev->no_command_memory) {
> >   		/*
> > -		 * VFs do no implement the memory enable bit of the COMMAND
> > -		 * register therefore we'll not have it set in our initial
> > -		 * copy of config space after pci_enable_device().  For
> > -		 * consistency with PFs, set the virtual enable bit here.
> > +		 * VFs and devices that set pdev->no_command_memory do not
> > +		 * implement the memory enable bit of the COMMAND register
> > +		 * therefore we'll not have it set in our initial copy of
> > +		 * config space after pci_enable_device().  For consistency
> > +		 * with PFs, set the virtual enable bit here.
> >   		 */
> >   		*(__le16 *)&vconfig[PCI_COMMAND] |=
> >   					cpu_to_le16(PCI_COMMAND_MEMORY);
> >   
>

Matthew Rosato Sept. 22, 2020, 4:57 p.m. UTC | #3

On 9/22/20 12:40 PM, Alex Williamson wrote:
> On Mon, 21 Sep 2020 08:43:29 -0400
> Matthew Rosato <[e-mail]> wrote:
> 
>> On 9/10/20 10:59 AM, Matthew Rosato wrote:
>>> While it is true that devices with is_virtfn=1 will have a Memory Space
>>> Enable bit that is hard-wired to 0, this is not the only case where we
>>> see this behavior -- For example some bare-metal hypervisors lack
>>> Memory Space Enable bit emulation for devices not setting is_virtfn
>>> (s390). Fix this by instead checking for the newly-added
>>> no_command_memory bit which directly denotes the need for
>>> PCI_COMMAND_MEMORY emulation in vfio.
>>>
>>> Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
>>> Signed-off-by: Matthew Rosato <[e-mail]>
>>> Reviewed-by: Niklas Schnelle <[e-mail]>
>>> Reviewed-by: Pierre Morel <[e-mail]>
>>
>> Polite ping on this patch as the other 2 have now received maintainer
>> ACKs or reviews.  I'm concerned about this popping up in distros as
>> abafbc551fdd was a CVE fix.  Related, see question from the cover:
>>
>> - Restored the fixes tag to patch 3 (but the other 2 patches are
>>     now pre-reqs -- cc stable 5.8?)
> 
> I've got these queued in my local branch which I'll push to next for
> v5.10.  I'm thinking that perhaps the right thing would be to add the
> fixes tag to all three patches, otherwise I could see that the PCI/VF
> change might get picked as a dependency, but not the s390 specific one.
> Does this sound correct to everyone?  Thanks,

Sounds good to me.  Thanks!

Pierre Morel Sept. 23, 2020, 8:51 a.m. UTC | #4

On 2020-09-22 18:40, Alex Williamson wrote:
> On Mon, 21 Sep 2020 08:43:29 -0400
> Matthew Rosato <[e-mail]> wrote:
> 
>> On 9/10/20 10:59 AM, Matthew Rosato wrote:
>>> While it is true that devices with is_virtfn=1 will have a Memory Space
>>> Enable bit that is hard-wired to 0, this is not the only case where we
>>> see this behavior -- For example some bare-metal hypervisors lack
>>> Memory Space Enable bit emulation for devices not setting is_virtfn
>>> (s390). Fix this by instead checking for the newly-added
>>> no_command_memory bit which directly denotes the need for
>>> PCI_COMMAND_MEMORY emulation in vfio.
>>>
>>> Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
>>> Signed-off-by: Matthew Rosato <[e-mail]>
>>> Reviewed-by: Niklas Schnelle <[e-mail]>
>>> Reviewed-by: Pierre Morel <[e-mail]>
>>
>> Polite ping on this patch as the other 2 have now received maintainer
>> ACKs or reviews.  I'm concerned about this popping up in distros as
>> abafbc551fdd was a CVE fix.  Related, see question from the cover:
>>
>> - Restored the fixes tag to patch 3 (but the other 2 patches are
>>     now pre-reqs -- cc stable 5.8?)
> 
> I've got these queued in my local branch which I'll push to next for
> v5.10.  I'm thinking that perhaps the right thing would be to add the
> fixes tag to all three patches, otherwise I could see that the PCI/VF
> change might get picked as a dependency, but not the s390 specific one.
> Does this sound correct to everyone?  Thanks,
> 
> Alex
> 
sound correct for me.
Thanks.

Pierre

Patch

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index d98843f..5076d01 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -406,7 +406,7 @@  bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
 	 * PF SR-IOV capability, there's therefore no need to trigger
 	 * faults based on the virtual value.
 	 */
-	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);
+	return pdev->no_command_memory || (cmd & PCI_COMMAND_MEMORY);
 }
 
 /*
@@ -520,8 +520,8 @@  static int vfio_basic_config_read(struct vfio_pci_device *vdev, int pos,
 
 	count = vfio_default_config_read(vdev, pos, count, perm, offset, val);
 
-	/* Mask in virtual memory enable for SR-IOV devices */
-	if (offset == PCI_COMMAND && vdev->pdev->is_virtfn) {
+	/* Mask in virtual memory enable */
+	if (offset == PCI_COMMAND && vdev->pdev->no_command_memory) {
 		u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
 		u32 tmp_val = le32_to_cpu(*val);
 
@@ -589,9 +589,11 @@  static int vfio_basic_config_write(struct vfio_pci_device *vdev, int pos,
 		 * shows it disabled (phys_mem/io, then the device has
 		 * undergone some kind of backdoor reset and needs to be
 		 * restored before we allow it to enable the bars.
-		 * SR-IOV devices will trigger this, but we catch them later
+		 * SR-IOV devices will trigger this - for mem enable let's
+		 * catch this now and for io enable it will be caught later
 		 */
-		if ((new_mem && virt_mem && !phys_mem) ||
+		if ((new_mem && virt_mem && !phys_mem &&
+		     !pdev->no_command_memory) ||
 		    (new_io && virt_io && !phys_io) ||
 		    vfio_need_bar_restore(vdev))
 			vfio_bar_restore(vdev);
@@ -1734,12 +1736,14 @@  int vfio_config_init(struct vfio_pci_device *vdev)
 				 vconfig[PCI_INTERRUPT_PIN]);
 
 		vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
-
+	}
+	if (pdev->no_command_memory) {
 		/*
-		 * VFs do no implement the memory enable bit of the COMMAND
-		 * register therefore we'll not have it set in our initial
-		 * copy of config space after pci_enable_device().  For
-		 * consistency with PFs, set the virtual enable bit here.
+		 * VFs and devices that set pdev->no_command_memory do not
+		 * implement the memory enable bit of the COMMAND register
+		 * therefore we'll not have it set in our initial copy of
+		 * config space after pci_enable_device().  For consistency
+		 * with PFs, set the virtual enable bit here.
 		 */
 		*(__le16 *)&vconfig[PCI_COMMAND] |=
 					cpu_to_le16(PCI_COMMAND_MEMORY);