[PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Alexey V. Vissarionov]

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]


Although unlikely, the 'id' value may be as big as 4294967295
(uint32_max) and "virtfn4294967295\0" would require 17 bytes
instead of 16.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: dd7cc44d0 ("PCI: add SR-IOV API for Physical Function driver")
Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
---
 drivers/pci/iov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-PCI-IOV-virtfn4294967295-0-requires-17-bytes.diff --]
[-- Type: text/x-patch; name="0001-PCI-IOV-virtfn4294967295-0-requires-17-bytes.diff", Size: 306 bytes --]

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9522175..ad54a07 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -14,7 +14,7 @@
 #include <linux/delay.h>
 #include "pci.h"
 
-#define VIRTFN_ID_LEN	16
+#define VIRTFN_ID_LEN	17
 
 int pci_iov_virtfn_bus(struct pci_dev *dev, int vf_id)
 {

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Krzysztof Wilczyński]

(CC Alex directly for visibility)

Hi Alexey,

Thank you for sending the patch over!  However, if possible, can you send
it as plain text without any multi-part MIME involved?  This is as per:

  https://www.kernel.org/doc/html/latest/process/submitting-patches.html

People have scripts to handle patches submissions, plus there is automation
that relies on e-mails being just plain text with inline patches.

Sorry for the trouble, and thank you for understanding.

> Although unlikely, the 'id' value may be as big as 4294967295
> (uint32_max) and "virtfn4294967295\0" would require 17 bytes
> instead of 16.

If possible, it would be nice to mention that this needed to make sure
that there is enough space to correctly NULL-terminate the ID string.

Thank you!

	Krzysztof

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Alexey V. Vissarionov]

On 2022-12-18 19:57:02 +0900, Krzysztof Wilczyński wrote:

 > Thank you for sending the patch over! However, if possible,
 > can you send it as plain text without any multi-part MIME
 > involved?

ACK.

 > If possible, it would be nice to mention that this needed
 > to make sure that there is enough space to correctly
 > NULL-terminate the ID string.

ACK.

So, here goes the corrected text:

Although unlikely, the 'id' value may be as big as 4294967295
(uint32_max) and "virtfn4294967295\0" would require 17 bytes
instead of 16 to make sure that buffer has enough space to
properly NULL-terminate the ID string.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: dd7cc44d0 ("PCI: add SR-IOV API for Physical Function driver")
Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>


diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9522175..ad54a07 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -14,7 +14,7 @@
 #include <linux/delay.h>
 #include "pci.h"
 
-#define VIRTFN_ID_LEN	16
+#define VIRTFN_ID_LEN	17
 
 int pci_iov_virtfn_bus(struct pci_dev *dev, int vf_id)
 {


-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Matthew Wilcox]

On Sun, Dec 18, 2022 at 03:21:39PM +0300, Alexey V. Vissarionov wrote:
> On 2022-12-18 19:57:02 +0900, Krzysztof Wilczyński wrote:
> 
>  > Thank you for sending the patch over! However, if possible,
>  > can you send it as plain text without any multi-part MIME
>  > involved?
> 
> ACK.
> 
>  > If possible, it would be nice to mention that this needed
>  > to make sure that there is enough space to correctly
>  > NULL-terminate the ID string.
> 
> ACK.
> 
> So, here goes the corrected text:
> 
> Although unlikely, the 'id' value may be as big as 4294967295
> (uint32_max) and "virtfn4294967295\0" would require 17 bytes
> instead of 16 to make sure that buffer has enough space to
> properly NULL-terminate the ID string.

Wait, what?  How can we get to a number that large for the virtual
function ID?  devfn is 8 bits, bus is a further 8 bits.  Sure, domain
is an extra 16 bits on top of that but I'm pretty sure that virtual
functions can't span multiple domains.  Unless that's changed recently?

Even if they can, we'd need to span 2^14 domains to get up to a billion
IDs.  That's a hell of a system and I think overflowing here is the
least of our problems.

So while this is typed as u32, I don't think it can get anywhere close.

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Alexey V. Vissarionov]

On 2022-12-18 22:19:24 +0000, Matthew Wilcox wrote:

 >>> Thank you for sending the patch over! However, if possible,
 >>> can you send it as plain text without any multi-part MIME
 >>> involved?
 >> ACK.
 >>> If possible, it would be nice to mention that this needed
 >>> to make sure that there is enough space to correctly
 >>> NULL-terminate the ID string.
 >> ACK.
 >> So, here goes the corrected text:
 >> Although unlikely, the 'id' value may be as big as 4294967295
 >> (uint32_max) and "virtfn4294967295\0" would require 17 bytes
 >> instead of 16 to make sure that buffer has enough space to
 >> properly NULL-terminate the ID string.
 > Wait, what? How can we get to a number that large for the
 > virtual function ID? devfn is 8 bits, bus is a further 8 bits.
 > Sure, domain is an extra 16 bits on top of that but I'm pretty
 > sure that virtual functions can't span multiple domains.
 > Unless that's changed recently? Even if they can, we'd need
 > to span 2^14 domains to get up to a billion IDs. That's a
 > hell of a system and I think overflowing here is the least
 > of our problems.

Possibly in some synthetic cases this may be achieved with some
specially crafted "BadPCI" (similar to "BadUSB") device...

 > So while this is typed as u32, I don't think it can get
 > anywhere close.

Anyway, the final decision is up to you.


-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Niklas Schnelle]

On Sun, 2022-12-18 at 22:19 +0000, Matthew Wilcox wrote:
> On Sun, Dec 18, 2022 at 03:21:39PM +0300, Alexey V. Vissarionov wrote:
> > On 2022-12-18 19:57:02 +0900, Krzysztof Wilczyński wrote:
> > 
> >  > Thank you for sending the patch over! However, if possible,
> >  > can you send it as plain text without any multi-part MIME
> >  > involved?
> > 
> > ACK.
> > 
> >  > If possible, it would be nice to mention that this needed
> >  > to make sure that there is enough space to correctly
> >  > NULL-terminate the ID string.
> > 
> > ACK.
> > 
> > So, here goes the corrected text:
> > 
> > Although unlikely, the 'id' value may be as big as 4294967295
> > (uint32_max) and "virtfn4294967295\0" would require 17 bytes
> > instead of 16 to make sure that buffer has enough space to
> > properly NULL-terminate the ID string.
> 
> Wait, what?  How can we get to a number that large for the virtual
> function ID?  devfn is 8 bits, bus is a further 8 bits.  Sure, domain
> is an extra 16 bits on top of that but I'm pretty sure that virtual
> functions can't span multiple domains.  Unless that's changed recently?
> 
> Even if they can, we'd need to span 2^14 domains to get up to a billion
> IDs.  That's a hell of a system and I think overflowing here is the
> least of our problems.
> 
> So while this is typed as u32, I don't think it can get anywhere close.

While we can't realistically get such high IDs on s390 at the moment
it's important to note that we do use the PCI domains differently and
will use the entire 16 bit namespace. As s390 aka mainframes always has
a virtualization layer which does the PCI enumeration for us and just
presents us with single PCI functions from which we reconstruct a PCI
hiearchy. Historically the only PCI devices available were Virtual
Functions which then use bus and devfn 0 with either an incrementing or
user defined domain ID as virtual PCI domain. Meaning that
"ffff:00:00.0" can and does occur. Since commit d1379279f2d6b
("s390/pci: Handling multifunctions") we may also present multiple
functions under the same domain with the a local topology matching the
hardware, though currently limited to a single fixed bus 0, but still
with a virtual and user defined domain ID. So at least on s390 we would
"only" need 2^16 VFs to see ffff:ff:ff.f.

That said, as far as I can tell this wouldn't actually create a virtfn
ID of such a high value as I don't think the domain actually plays into
that. For example with the following (sub) topology of 2 PFs with 4 VFs
each:

 +-[0008:00]-+-00.0  Mellanox Technologies MT28800 Family [ConnectX-5 Ex]
 |           +-00.1  Mellanox Technologies MT28800 Family [ConnectX-5 Ex]
 |           +-00.2  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]
 |           +-00.3  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]
 |           +-00.4  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]
 |           +-00.5  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]
 |           +-08.2  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]
 |           +-08.3  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]
 |           +-08.4  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]
 |           \-08.5  Mellanox Technologies MT28800 Family [ConnectX-5 Ex Virtual Function]

I actually get:

# ls -l /sys/bus/pci/devices/0008\:00\:00.0/virtfn*
... /sys/bus/pci/devices/0008:00:00.0/virtfn0 -> ../0008:00:00.2
... /sys/bus/pci/devices/0008:00:00.0/virtfn1 -> ../0008:00:00.3
... /sys/bus/pci/devices/0008:00:00.0/virtfn2 -> ../0008:00:00.4
... /sys/bus/pci/devices/0008:00:00.0/virtfn3 -> ../0008:00:00.5

# ls -l /sys/bus/pci/devices/0008\:00\:00.1/virtfn*
... /sys/bus/pci/devices/0008:00:00.1/virtfn0 -> ../0008:00:08.2
... /sys/bus/pci/devices/0008:00:00.1/virtfn1 -> ../0008:00:08.3
... /sys/bus/pci/devices/0008:00:00.1/virtfn2 -> ../0008:00:08.4
... /sys/bus/pci/devices/0008:00:00.1/virtfn3 -> ../0008:00:08.5

Thanks,
Niklas

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Bjorn Helgaas]

On Sun, Dec 18, 2022 at 10:19:24PM +0000, Matthew Wilcox wrote:
> On Sun, Dec 18, 2022 at 03:21:39PM +0300, Alexey V. Vissarionov wrote:
> > On 2022-12-18 19:57:02 +0900, Krzysztof Wilczyński wrote:
> ...

> > Although unlikely, the 'id' value may be as big as 4294967295
> > (uint32_max) and "virtfn4294967295\0" would require 17 bytes
> > instead of 16 to make sure that buffer has enough space to
> > properly NULL-terminate the ID string.
> 
> Wait, what?  How can we get to a number that large for the virtual
> function ID?  devfn is 8 bits, bus is a further 8 bits.  Sure, domain
> is an extra 16 bits on top of that but I'm pretty sure that virtual
> functions can't span multiple domains.  Unless that's changed recently?
> 
> Even if they can, we'd need to span 2^14 domains to get up to a billion
> IDs.  That's a hell of a system and I think overflowing here is the
> least of our problems.
> 
> So while this is typed as u32, I don't think it can get anywhere close.

Is there an argument *against* this patch (as opposed to "this is
probably unnecessary and it requires a lot of analysis to prove that
we don't need it")?

My biggest concern here is that there's no connection between the
VIRTFN_ID_LEN definition and the use.  Even a comment about how the
value of 16 or 17 was derived would help.

Bjorn

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Matthew Wilcox]

On Thu, Dec 29, 2022 at 12:12:58PM -0600, Bjorn Helgaas wrote:
> On Sun, Dec 18, 2022 at 10:19:24PM +0000, Matthew Wilcox wrote:
> > On Sun, Dec 18, 2022 at 03:21:39PM +0300, Alexey V. Vissarionov wrote:
> > > On 2022-12-18 19:57:02 +0900, Krzysztof Wilczyński wrote:
> > ...
> 
> > > Although unlikely, the 'id' value may be as big as 4294967295
> > > (uint32_max) and "virtfn4294967295\0" would require 17 bytes
> > > instead of 16 to make sure that buffer has enough space to
> > > properly NULL-terminate the ID string.
> > 
> > Wait, what?  How can we get to a number that large for the virtual
> > function ID?  devfn is 8 bits, bus is a further 8 bits.  Sure, domain
> > is an extra 16 bits on top of that but I'm pretty sure that virtual
> > functions can't span multiple domains.  Unless that's changed recently?
> > 
> > Even if they can, we'd need to span 2^14 domains to get up to a billion
> > IDs.  That's a hell of a system and I think overflowing here is the
> > least of our problems.
> > 
> > So while this is typed as u32, I don't think it can get anywhere close.
> 
> Is there an argument *against* this patch (as opposed to "this is
> probably unnecessary and it requires a lot of analysis to prove that
> we don't need it")?

It consumes additional stack space?  It's an example of changing code
to shut up a tool that is of dubious value?

Re: [PATCH] PCI/IOV: "virtfn4294967295\0" requires 17 bytes

[Bjorn Helgaas]

On Sun, Dec 18, 2022 at 03:21:39PM +0300, Alexey V. Vissarionov wrote:
> On 2022-12-18 19:57:02 +0900, Krzysztof Wilczyński wrote:
> 
>  > Thank you for sending the patch over! However, if possible,
>  > can you send it as plain text without any multi-part MIME
>  > involved?
> 
> ACK.
> 
>  > If possible, it would be nice to mention that this needed
>  > to make sure that there is enough space to correctly
>  > NULL-terminate the ID string.
> 
> ACK.
> 
> So, here goes the corrected text:
> 
> Although unlikely, the 'id' value may be as big as 4294967295
> (uint32_max) and "virtfn4294967295\0" would require 17 bytes
> instead of 16 to make sure that buffer has enough space to
> properly NULL-terminate the ID string.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: dd7cc44d0 ("PCI: add SR-IOV API for Physical Function driver")
> Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>

I collected this up and applied to pci/iov for v6.3 as below.  I agree
this is probably only a theoretical issue, but it's easier to spend a
byte of stack space than to prove that we don't need to.

Bjorn


commit 58d4c63d0a27 ("PCI/IOV: Enlarge virtfn sysfs name buffer")
parent 1b929c02afd3
Author: Alexey V. Vissarionov <gremlin@altlinux.org>
Date:   Sun Dec 18 06:33:47 2022 +0300

    PCI/IOV: Enlarge virtfn sysfs name buffer
    
    The sysfs link name "virtfn%u" constructed by pci_iov_sysfs_link() requires
    17 bytes to contain the longest possible string.  Increase VIRTFN_ID_LEN to
    accommodate that.
    
    Found by Linux Verification Center (linuxtesting.org) with SVACE.
    
    [bhelgaas: commit log, comment at #define]
    Fixes: dd7cc44d0 ("PCI: add SR-IOV API for Physical Function driver")
    Link: https://lore.kernel.org/r/20221218033347.23743-1-gremlin@altlinux.org
    Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 952217572113..b2e8322755c1 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -14,7 +14,7 @@
 #include <linux/delay.h>
 #include "pci.h"
 
-#define VIRTFN_ID_LEN	16
+#define VIRTFN_ID_LEN	17	/* "virtfn%u\0" for 2^32 - 1 */
 
 int pci_iov_virtfn_bus(struct pci_dev *dev, int vf_id)
 {