[PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Yuval Shaia]

Guest driver allocates and initialize page tables to be used as a ring
of descriptors for CQ and async events.
The page table that represents the ring, along with the number of pages
in the page table is passed to the device.
Currently our device supports only one page table for a ring.

Let's make sure that the number of page table entries the driver
reports, do not exceeds the one page table size.

Reported-by: Soul Chen <soulchen8650@gmail.com>
Signed-off-by: Yuval Shaia <yuval.shaia.ml@gmail.com>
---
v0 -> v1:
	* Take ring-state into account
	* Add Reported-by
---
 hw/rdma/vmw/pvrdma_main.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 4fc6712025..55b338046e 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -91,19 +91,33 @@ static int init_dev_ring(PvrdmaRing *ring, PvrdmaRingState **ring_state,
                          dma_addr_t dir_addr, uint32_t num_pages)
 {
     uint64_t *dir, *tbl;
-    int rc = 0;
+    int max_pages, rc = 0;
 
     if (!num_pages) {
         rdma_error_report("Ring pages count must be strictly positive");
         return -EINVAL;
     }
 
+    /*
+     * Make sure we can satisfy the requested number of pages in a single
+     * TARGET_PAGE_SIZE sized page table (taking into account that first entry
+     * is reserved for ring-state)
+     */
+    max_pages = TARGET_PAGE_SIZE / sizeof(dma_addr_t) - 1;
+    if (num_pages > max_pages) {
+        rdma_error_report("Maximum pages on a single directory must not exceed %d\n",
+                          max_pages);
+        return -EINVAL;
+    }
+
     dir = rdma_pci_dma_map(pci_dev, dir_addr, TARGET_PAGE_SIZE);
     if (!dir) {
         rdma_error_report("Failed to map to page directory (ring %s)", name);
         rc = -ENOMEM;
         goto out;
     }
+
+    /* We support only one page table for a ring */
     tbl = rdma_pci_dma_map(pci_dev, dir[0], TARGET_PAGE_SIZE);
     if (!tbl) {
         rdma_error_report("Failed to map to page table (ring %s)", name);
-- 
2.20.1

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Michael Tokarev]

01.03.2023 17:29, Yuval Shaia wrote:
> Guest driver allocates and initialize page tables to be used as a ring
> of descriptors for CQ and async events.
> The page table that represents the ring, along with the number of pages
> in the page table is passed to the device.
> Currently our device supports only one page table for a ring.
> 
> Let's make sure that the number of page table entries the driver
> reports, do not exceeds the one page table size.

Ping? This seems to has been missing..

/mjt

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Michael Tokarev]

10.04.2023 20:48, Michael Tokarev wrote:
> 01.03.2023 17:29, Yuval Shaia wrote:
>> Guest driver allocates and initialize page tables to be used as a ring
>> of descriptors for CQ and async events.
>> The page table that represents the ring, along with the number of pages
>> in the page table is passed to the device.
>> Currently our device supports only one page table for a ring.
>>
>> Let's make sure that the number of page table entries the driver
>> reports, do not exceeds the one page table size.
> 
> Ping? This seems to has been missing..

This is CVE-2023-1544, which can be mentioned in the subject line at
patch apply.

/mjt

Re: [PATCH-for-8.0? v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Philippe Mathieu-Daudé]

On 1/3/23 15:29, Yuval Shaia wrote:
> Guest driver allocates and initialize page tables to be used as a ring
> of descriptors for CQ and async events.
> The page table that represents the ring, along with the number of pages
> in the page table is passed to the device.
> Currently our device supports only one page table for a ring.
> 
> Let's make sure that the number of page table entries the driver
> reports, do not exceeds the one page table size.
> 

Fixes: CVE-2023-1544

> Reported-by: Soul Chen <soulchen8650@gmail.com>
> Signed-off-by: Yuval Shaia <yuval.shaia.ml@gmail.com>
> ---
> v0 -> v1:
> 	* Take ring-state into account
> 	* Add Reported-by
> ---
>   hw/rdma/vmw/pvrdma_main.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 4fc6712025..55b338046e 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -91,19 +91,33 @@ static int init_dev_ring(PvrdmaRing *ring, PvrdmaRingState **ring_state,
>                            dma_addr_t dir_addr, uint32_t num_pages)
>   {
>       uint64_t *dir, *tbl;
> -    int rc = 0;
> +    int max_pages, rc = 0;
>   
>       if (!num_pages) {
>           rdma_error_report("Ring pages count must be strictly positive");
>           return -EINVAL;
>       }
>   
> +    /*
> +     * Make sure we can satisfy the requested number of pages in a single
> +     * TARGET_PAGE_SIZE sized page table (taking into account that first entry
> +     * is reserved for ring-state)
> +     */

Worth a definition? Or maybe better an enum.

> +    max_pages = TARGET_PAGE_SIZE / sizeof(dma_addr_t) - 1;

Possibly clearer as a #define in pvrdma_dev_ring.h.

> +    if (num_pages > max_pages) {
> +        rdma_error_report("Maximum pages on a single directory must not exceed %d\n",
> +                          max_pages);
> +        return -EINVAL;
> +    }
> +
>       dir = rdma_pci_dma_map(pci_dev, dir_addr, TARGET_PAGE_SIZE);
>       if (!dir) {
>           rdma_error_report("Failed to map to page directory (ring %s)", name);
>           rc = -ENOMEM;
>           goto out;
>       }
> +
> +    /* We support only one page table for a ring */
>       tbl = rdma_pci_dma_map(pci_dev, dir[0], TARGET_PAGE_SIZE);
>       if (!tbl) {
>           rdma_error_report("Failed to map to page table (ring %s)", name);

Now looking at the following pvrdma_ring_init() call, I see too many
magic values for my taste, so feel unsafe to review:

     /* RX ring is the second */
     (*ring_state)++;
     rc = pvrdma_ring_init(ring, name, pci_dev,
                           (PvrdmaRingState *)*ring_state,
                           (num_pages - 1) * TARGET_PAGE_SIZE /
                           sizeof(struct pvrdma_cqne),
                           sizeof(struct pvrdma_cqne),
                           (dma_addr_t *)&tbl[1],
                           (dma_addr_t)num_pages - 1);

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Michael Tokarev]

01.03.2023 17:29, Yuval Shaia wrote:
> Guest driver allocates and initialize page tables to be used as a ring
> of descriptors for CQ and async events.
> The page table that represents the ring, along with the number of pages
> in the page table is passed to the device.
> Currently our device supports only one page table for a ring.
> 
> Let's make sure that the number of page table entries the driver
> reports, do not exceeds the one page table size.
> 
> Reported-by: Soul Chen <soulchen8650@gmail.com>
> Signed-off-by: Yuval Shaia <yuval.shaia.ml@gmail.com>
> ---
> v0 -> v1:
> 	* Take ring-state into account
> 	* Add Reported-by
> ---
>   hw/rdma/vmw/pvrdma_main.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)

Fixes: CVE-2023-1544

Ping ^2?
Laurent, maybe you can take this one too?
I understand the fact you picked up the previous one in this area
does not make you pvrdma maintainer, but it is definitely being stuck.. :)

/mjt

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Mauro Matteo Cascella]

On Mon, May 15, 2023 at 6:13 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 01.03.2023 17:29, Yuval Shaia wrote:
> > Guest driver allocates and initialize page tables to be used as a ring
> > of descriptors for CQ and async events.
> > The page table that represents the ring, along with the number of pages
> > in the page table is passed to the device.
> > Currently our device supports only one page table for a ring.
> >
> > Let's make sure that the number of page table entries the driver
> > reports, do not exceeds the one page table size.
> >
> > Reported-by: Soul Chen <soulchen8650@gmail.com>
> > Signed-off-by: Yuval Shaia <yuval.shaia.ml@gmail.com>
> > ---
> > v0 -> v1:
> >       * Take ring-state into account
> >       * Add Reported-by
> > ---
> >   hw/rdma/vmw/pvrdma_main.c | 16 +++++++++++++++-
> >   1 file changed, 15 insertions(+), 1 deletion(-)
>
> Fixes: CVE-2023-1544
>
> Ping ^2?

Ping ^3?

> Laurent, maybe you can take this one too?
> I understand the fact you picked up the previous one in this area
> does not make you pvrdma maintainer, but it is definitely being stuck.. :)
>
> /mjt
>


-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Red Hat Product Security]

[-- Attachment #1.1: Type: text/plain, Size: 679 bytes --]

Hello!

INC2534320 ([PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver) has been updated.

Opened for: yuval.shaia.ml@gmail.com
Followers: qemu-devel@nongnu.org, soulchen8650@gmail.com, Mauro Matteo Cascella, qemu-security@nongnu.org, yuval.shaia.ml@gmail.com, marcel.apfelbaum@gmail.com

A Guest updated your request with the following comments:

Reply from: pjp@fedoraproject.org [mailto:pjp@fedoraproject.org]
 Hello Yuval,

How can I track and update my request?

To respond, reply to this email. You may also create a new email and include the request number (INC2534320) in the subject.

Thank you,
Product Security

Ref:MSG71528958

[-- Attachment #1.2: Type: text/html, Size: 1309 bytes --]

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Yuval Shaia]

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

Hi,
Patch is currently under review.
From my end, it was tested and proved to solve the problem.

To follow up you may need to check qemu-devel@nongnu.org from time to time.

Marcel, any feedback?

Yuval

On Mon, 13 Mar 2023 at 18:56, Red Hat Product Security <secalert@redhat.com>
wrote:

> Hello!
>
> INC2534320 ([PATCH v1] hw/pvrdma: Protect against buggy or malicious guest
> driver) has been updated.
>
> Opened for: yuval.shaia.ml@gmail.com
> Followers: qemu-devel@nongnu.org, soulchen8650@gmail.com, Mauro Matteo
> Cascella, qemu-security@nongnu.org, yuval.shaia.ml@gmail.com,
> marcel.apfelbaum@gmail.com
>
> A Guest updated your request with the following comments:
> Reply from: pjp@fedoraproject.org
>
> Hello Yuval,
>
> *How can I track and update my request?*
>
> To respond, reply to this email. You may also create a new email and
> include the request number (INC2534320) in the subject.
>
> Thank you,
> Product Security
>
> Ref:MSG71528958
>

[-- Attachment #2: Type: text/html, Size: 2356 bytes --]

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Mauro Matteo Cascella]

Hi Yuval,

Dropping <qemu-security> and <secalert@redhat.com>. This is CVE-2023-1544.

The patch looks good to me. Thank you.

On Mon, Mar 20, 2023 at 1:07 PM Yuval Shaia <yuval.shaia.ml@gmail.com> wrote:
>
> Hi,
> Patch is currently under review.
> From my end, it was tested and proved to solve the problem.
>
> To follow up you may need to check qemu-devel@nongnu.org from time to time.
>
> Marcel, any feedback?

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Red Hat Product Security]

[-- Attachment #1.1: Type: text/plain, Size: 1961 bytes --]

Hello!

INC2534320 ([PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver) has been updated.

Opened for: yuval.shaia.ml@gmail.com
Followers: qemu-devel@nongnu.org, soulchen8650@gmail.com, Mauro Matteo Cascella, qemu-security@nongnu.org, yuval.shaia.ml@gmail.com, marcel.apfelbaum@gmail.com

A Guest updated your request with the following comments:

Reply from: yuval.shaia.ml@gmail.com [mailto:yuval.shaia.ml@gmail.com]
 Hi,
Patch is currently under review.
From my end, it was tested and proved to solve the problem.
 To follow up you may need to check qemu-devel@nongnu.org [mailto:qemu-devel@nongnu.org] from time to time.
 Marcel, any feedback?
 Yuval
 On Mon, 13 Mar 2023 at 18:56, Red Hat Product Security <secalert@redhat.com [mailto:secalert@redhat.com]>
wrote:
 > Hello!
>
> INC2534320 ([PATCH v1] hw/pvrdma: Protect against buggy or malicious guest
> driver) has been updated.
>
> Opened for: yuval.shaia.ml@gmail.com [mailto:yuval.shaia.ml@gmail.com]
> Followers: qemu-devel@nongnu.org [mailto:qemu-devel@nongnu.org], soulchen8650@gmail.com [mailto:soulchen8650@gmail.com], Mauro Matteo
> Cascella, qemu-security@nongnu.org [mailto:qemu-security@nongnu.org], yuval.shaia.ml@gmail.com [mailto:yuval.shaia.ml@gmail.com],
> marcel.apfelbaum@gmail.com [mailto:marcel.apfelbaum@gmail.com]
>
> A Guest updated your request with the following comments:
> Reply from: pjp@fedoraproject.org [mailto:pjp@fedoraproject.org]
>
> Hello Yuval,
>
> *How can I track and update my request?*
>
> To respond, reply to this email. You may also create a new email and
> include the request number (INC2534320) in the subject.
>
> Thank you,
> Product Security
>
> Ref:MSG71528958
>

How can I track and update my request?

To respond, reply to this email. You may also create a new email and include the request number (INC2534320) in the subject.

Thank you,
Product Security

Ref:MSG71828656

[-- Attachment #1.2: Type: text/html, Size: 3606 bytes --]

Re: [PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver

[Red Hat Product Security]

[-- Attachment #1.1: Type: text/plain, Size: 1760 bytes --]

Hello!

INC2534320 ([PATCH v1] hw/pvrdma: Protect against buggy or malicious guest driver) has been closed.

Opened for: yuval.shaia.ml@gmail.com
Followers: qemu-devel@nongnu.org, soulchen8650@gmail.com, Mauro Matteo Cascella, qemu-security@nongnu.org, yuval.shaia.ml@gmail.com, marcel.apfelbaum@gmail.com

A Guest updated your request with the following comments:

Reply from: mjt@tls.msk.ru [mailto:mjt@tls.msk.ru]
 01.03.2023 17:29, Yuval Shaia wrote:
> Guest driver allocates and initialize page tables to be used as a ring
> of descriptors for CQ and async events.
> The page table that represents the ring, along with the number of pages
> in the page table is passed to the device.
> Currently our device supports only one page table for a ring.
> 
> Let's make sure that the number of page table entries the driver
> reports, do not exceeds the one page table size.
> 
> Reported-by: Soul Chen <soulchen8650@gmail.com [mailto:soulchen8650@gmail.com]>
> Signed-off-by: Yuval Shaia <yuval.shaia.ml@gmail.com [mailto:yuval.shaia.ml@gmail.com]>
> ---
> v0 -> v1:
> * Take ring-state into account
> * Add Reported-by
> ---
> hw/rdma/vmw/pvrdma_main.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
 Fixes: CVE-2023-1544
 Ping ^2?
Laurent, maybe you can take this one too?
I understand the fact you picked up the previous one in this area
does not make you pvrdma maintainer, but it is definitely being stuck.. :)
 /mjt

How can I track and update my request?

We want to make sure we have provided you with a complete resolution.

If you feel that your request should not be closed yet, then please reply to this email and let us know.

Thank you,
Product Security

Ref:MSG74315854

[-- Attachment #1.2: Type: text/html, Size: 2896 bytes --]