diff mbox series

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

Message ID 20230301142926.18686-1-yuval.shaia.ml@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v1] hw/pvrdma: Protect against buggy or malicious guest driver | expand

Commit Message

Yuval Shaia March 1, 2023, 2:29 p.m. UTC
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(-)

Comments

Michael Tokarev April 10, 2023, 5:48 p.m. UTC | #1
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
Michael Tokarev April 10, 2023, 6:02 p.m. UTC | #2
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
Philippe Mathieu-Daudé April 11, 2023, 10:33 a.m. UTC | #3
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);
Michael Tokarev May 15, 2023, 4:13 p.m. UTC | #4
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
Mauro Matteo Cascella May 29, 2023, 9:52 a.m. UTC | #5
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
>
diff mbox series

Patch

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);