Fixes on top of CVE-2014-8159 kernel: infiniband: uverbs: unprotected physical memory access

PATCH In review Yann Droneaud 5 participants 7 messages Apr 13, 2015 – Apr 14, 2015 3 messages not in the archive InfiniBand

lore

Share & tools

lore

Yann Droneaud

author

2015-04-13 12:56 UTC #0 lore raw

cover Fixes on top of CVE-2014-8159 kernel: infiniband: uverbs: unprotected physical memory access

Hi,

Please find one patch to prevent a possible issue partially addressed by commit 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic") (see discussions in [1]) and another one to add back the possibility of registering memory mapped at 0 (which is probably not something to be allowed, but it's probably not up to ib_umem_get() to prevent it).

Changes from v0 [2]:
- don't touch to overflow logic in first patch:
not modifying the logic here so that the patch can be applied even on kernel without the overflow preventing checks, and second patch is going to rewrite the check.

- don't break overflow detection in second patch:
changing less or equal to less comparison broke the overflow detection logic regarding to rounding done by PAGE_ALIGN, so fixes this by checking for overflow in addr + size, then by checking for overflow in PAGE_ALIGN(addr + size).

[1] "Re: CVE-2014-8159 kernel: infiniband: uverbs: unprotected physical memory access"

http://mid.gmane.org/[email protected]
http://marc.info/[email protected]

[2] [PATCH RESEND 0/2] Fixes on top of CVE-2014-8159 kernel: infiniband: uverbs: unprotected physical memory access

http://mid.gmane.org/[email protected]
http://marc.info/[email protected]

Yann Droneaud (2):
IB/core: disallow registering 0-sized memory region IB/core: don't disallow registering region starting at 0x0

 drivers/infiniband/core/umem.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Signature · 1 line

2.1.0

A message that is not in the archive (sent off-list or not yet received): cover.1428929103.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud

2015-04-13 12:56 UTC #2 lore raw

PATCH 1/2 IB/core: disallow registering 0-sized memory region

If ib_umem_get() is called with a size equal to 0 and an non-page aligned address, one page will be pinned and a 0-sized umem will be returned to the caller.

This should not be allowed: it's not expected for a memory region to have a size equal to 0.

This patch adds a check to explicitly refuse to register a 0-sized region.

Link http://mid.gmane.org/cover.1428929103.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Cc <[email protected]>

Cc Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Jack Morgenstein <jackm-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Yann Droneaud <[email protected]>

---
 drivers/infiniband/core/umem.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/infiniband/core/umem.c +3 -0

index 8c014b5dab4c..9ac4068d2088 100644
@@ -99,6 +99,9 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 	if (dmasync)
 		dma_set_attr(DMA_ATTR_WRITE_BARRIER, &attrs);
 
+	if (!size)
+		return ERR_PTR(-EINVAL);
+
 	/*
 	 * If the combination of the addr and size requested for this memory
 	 * region causes an integer overflow, return error.

Signature · 6 lines

2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yann Droneaud

author

2015-04-13 12:56 UTC #3 lore raw

PATCH 2/2 IB/core: don't disallow registering region starting at 0x0

In reply to Yann Droneaud

In a call to ib_umem_get(), if address is 0x0 and size is already page aligned, check added in commit 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic") will refuse to register a memory region that could otherwise be valid (provided vm.mmap_min_addr sysctl and mmap_low_allowed SELinux knobs allow userspace to map something at address 0x0).

This patch allows back such registration: ib_umem_get() should probably don't care of the base address provided it can be pinned with get_user_pages().

There's two possible overflows, in (addr + size) and in PAGE_ALIGN(addr + size), this patch keep ensuring none of them happen while allowing to pin memory at address 0x0. Anyway, the case of size equal 0 is no more (partially) handled as 0-length memory region are disallowed by an earlier check.

Link http://mid.gmane.org/[email protected]

Cc <[email protected]> # 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic")

Cc Shachar Raindel <[email protected]>

Cc Jack Morgenstein <[email protected]>

Cc Or Gerlitz <[email protected]>

Signed-off-by Yann Droneaud <[email protected]>

---
 drivers/infiniband/core/umem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/core/umem.c +2 -2

index 9ac4068d2088..38acb3cfc545 100644
@@ -106,8 +106,8 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 	 * If the combination of the addr and size requested for this memory
 	 * region causes an integer overflow, return error.
 	 */
-	if ((PAGE_ALIGN(addr + size) <= size) ||
-	    (PAGE_ALIGN(addr + size) <= addr))
+	if (((addr + size) < addr) ||
+	    PAGE_ALIGN(addr + size) < (addr + size))
 		return ERR_PTR(-EINVAL);
 
 	if (!can_do_mlock())

Signature · 1 line

2.1.0

Sagi Grimberg

2015-04-14 09:20 UTC #4 lore raw

Re: [PATCH v1 2/2] IB/core: don't disallow registering region starting at 0x0

Quoted 34 lines · Yann Droneaud

In a call to ib_umem_get(), if address is 0x0 and size is already page aligned, check added in commit 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic") will refuse to register a memory region that could otherwise be valid (provided vm.mmap_min_addr sysctl and mmap_low_allowed SELinux knobs allow userspace to map something at address 0x0).
This patch allows back such registration: ib_umem_get() should probably don't care of the base address provided it can be pinned with get_user_pages().
There's two possible overflows, in (addr + size) and in PAGE_ALIGN(addr + size), this patch keep ensuring none of them happen while allowing to pin memory at address 0x0. Anyway, the case of size equal 0 is no more (partially) handled as 0-length memory region are disallowed by an earlier check.
Link http://mid.gmane.org/[email protected]
Cc <[email protected]> # 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic")
Cc Shachar Raindel <[email protected]>
Cc Jack Morgenstein <[email protected]>
Cc Or Gerlitz <[email protected]>
Signed-off-by Yann Droneaud <[email protected]>
---
  drivers/infiniband/core/umem.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/core/umem.c +0 -0
index 9ac4068d2088..38acb3cfc545 100644

drivers/infiniband/core/umem.c struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, · line 110

Quoted 7 lines

drivers/infiniband/core/umem.c +2 -2

@@ -106,8 +106,8 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
  	 * If the combination of the addr and size requested for this memory
  	 * region causes an integer overflow, return error.
  	 */
-	if ((PAGE_ALIGN(addr + size) <= size) ||
-	    (PAGE_ALIGN(addr + size) <= addr))
+	if (((addr + size) < addr) ||
+	    PAGE_ALIGN(addr + size) < (addr + size))

If you do change the first statement to be: (addr + size) <= addr wouldn't it cover patch #1?

Sagi.

Yann Droneaud

author

2015-04-14 12:00 UTC #5 lore raw

Re: [PATCH v1 2/2] IB/core: don't disallow registering region starting at 0x0

Hi,

Quoted 30 lines · Sagi Grimberg

Quoted 29 lines · Yann Droneaud
In a call to ib_umem_get(), if address is 0x0 and size is already page aligned, check added in commit 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic") will refuse to register a memory region that could otherwise be valid (provided vm.mmap_min_addr sysctl and mmap_low_allowed SELinux knobs allow userspace to map something at address 0x0).
This patch allows back such registration: ib_umem_get() should probably don't care of the base address provided it can be pinned with get_user_pages().
There's two possible overflows, in (addr + size) and in PAGE_ALIGN(addr + size), this patch keep ensuring none of them happen while allowing to pin memory at address 0x0. Anyway, the case of size equal 0 is no more (partially) handled as 0-length memory region are disallowed by an earlier check.
Link http://mid.gmane.org/[email protected]
Cc <[email protected]> # 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic")
Cc Shachar Raindel <[email protected]>
Cc Jack Morgenstein <[email protected]>
Cc Or Gerlitz <[email protected]>
Signed-off-by Yann Droneaud <[email protected]>
---
  drivers/infiniband/core/umem.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 16 lines

drivers/infiniband/core/umem.c struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, · line 110
Quoted 12 lines
drivers/infiniband/core/umem.c +2 -2
index 9ac4068d2088..38acb3cfc545 100644
@@ -106,8 +106,8 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
  	 * If the combination of the addr and size requested for this memory
  	 * region causes an integer overflow, return error.
  	 */
-	if ((PAGE_ALIGN(addr + size) <= size) ||
-	    (PAGE_ALIGN(addr + size) <= addr))
+	if (((addr + size) < addr) ||
+	    PAGE_ALIGN(addr + size) < (addr + size))
If you do change the first statement to be: (addr + size) <= addr wouldn't it cover patch #1?

Yes, but it doesn't sound a great place to do it: here it's about overflow, so I'd prefer not doing the null memory region check there.

Regards.

Signature · 2 lines

Yann Droneaud
OPTEYA

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sagi Grimberg

Reviewed-by

2015-04-14 12:50 UTC #7 lore raw

Re: [PATCH v1 2/2] IB/core: don't disallow registering region starting at 0x0

Quoted 50 lines · Yann Droneaud

Hi,
Quoted 30 lines · Sagi Grimberg
Quoted 29 lines · Yann Droneaud
In a call to ib_umem_get(), if address is 0x0 and size is already page aligned, check added in commit 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic") will refuse to register a memory region that could otherwise be valid (provided vm.mmap_min_addr sysctl and mmap_low_allowed SELinux knobs allow userspace to map something at address 0x0).
This patch allows back such registration: ib_umem_get() should probably don't care of the base address provided it can be pinned with get_user_pages().
There's two possible overflows, in (addr + size) and in PAGE_ALIGN(addr + size), this patch keep ensuring none of them happen while allowing to pin memory at address 0x0. Anyway, the case of size equal 0 is no more (partially) handled as 0-length memory region are disallowed by an earlier check.
Link http://mid.gmane.org/cover.1428929103.git.ydroneaud-RlY5vtjFyJ1hl2p70BpVqQ@public.gmane.orgm
Cc <[email protected]> # 8494057ab5e4 ("IB/uverbs: Prevent integer overflow in ib_umem_get address arithmetic")
Cc Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc Jack Morgenstein <jackm-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Yann Droneaud <[email protected]>
---
   drivers/infiniband/core/umem.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 16 lines
drivers/infiniband/core/umem.c struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, · line 110
Quoted 12 lines
drivers/infiniband/core/umem.c +2 -2
index 9ac4068d2088..38acb3cfc545 100644
@@ -106,8 +106,8 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
   	 * If the combination of the addr and size requested for this memory
   	 * region causes an integer overflow, return error.
   	 */
-	if ((PAGE_ALIGN(addr + size) <= size) ||
-	    (PAGE_ALIGN(addr + size) <= addr))
+	if (((addr + size) < addr) ||
+	    PAGE_ALIGN(addr + size) < (addr + size))
If you do change the first statement to be: (addr + size) <= addr wouldn't it cover patch #1?

Quoted 5 lines

Yes, but it doesn't sound a great place to do it: here it's about overflow, so I'd prefer not doing the null memory region check there.
Regards.

Sounds reasonable to me.

Reviewed-by Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Let's poke Shachar/Haggai to comment/approve on this as well.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 552D0D2A.8000604-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org

Haggai Eran

Reviewed-by

2015-04-14 14:35 UTC #9 lore raw

Re: [PATCH v1 2/2] IB/core: don't disallow registering region starting at 0x0

Quoted 3 lines · Sagi Grimberg

Quoted 2 lines · Yann Droneaud
Quoted 1 line · Sagi Grimberg
On 4/13/2015 3:56 PM, Yann Droneaud wrote:

...

Quoted 18 lines

Quoted 18 lines
Quoted 17 lines
drivers/infiniband/core/umem.c struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, · line 110
Quoted 13 lines
drivers/infiniband/core/umem.c +2 -2
index 9ac4068d2088..38acb3cfc545 100644
@@ -106,8 +106,8 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
        * If the combination of the addr and size requested for this memory
        * region causes an integer overflow, return error.
        */
-    if ((PAGE_ALIGN(addr + size) <= size) ||
-        (PAGE_ALIGN(addr + size) <= addr))
+    if (((addr + size) < addr) ||
+        PAGE_ALIGN(addr + size) < (addr + size))
If you do change the first statement to be: (addr + size) <= addr wouldn't it cover patch #1?

Quoted 7 lines

Quoted 5 lines
Yes, but it doesn't sound a great place to do it: here it's about overflow, so I'd prefer not doing the null memory region check there.
Regards.
Sounds reasonable to me.

Me too. As long as we prevent the integer overflow, there is no need to disallow regions starting at 0x0.

Reviewed-by Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]>

Quoted 5 lines

Reviewed-by Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>
Let's poke Shachar/Haggai to comment/approve on this as well.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html