[PATCH] RDMA/i40iw: Fix a mmap handler exploitation

[PATCH] RDMA/i40iw: Fix a mmap handler exploitation

[zhudi]

From: Di Zhu <zhudi21@huawei.com>

Notice that i40iw_mmap() is used as mmap for file
/dev/infiniband/uverbs%d and these files have access mode
with 0666 specified by uverbs_devnode() and vma->vm_pgoff
is directly used to calculate pfn which is passed in
remap_pfn_range function without any valid validation.

This would result in a malicious process being able to pass an arbitrary
physical address to ‘mmap’ which would allow for access to all of
kernel memory from user space and eventually gain root privileges.

So, we should check whether final calculated value of vm_pgoff is
in range of specified pci resource before we use it to calculate
pfn which is passed in remap_pfn_range

Signed-off-by: Di Zhu <zhudi21@huawei.com>
---
 drivers/infiniband/hw/i40iw/i40iw_verbs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
index 5ad381800f4d..7ec8f221eadb 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
@@ -185,6 +185,10 @@ static int i40iw_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
 
 	vma->vm_pgoff += db_addr_offset >> PAGE_SHIFT;
 
+	if (vma->vm_pgoff >
+		pci_resource_len(ucontext->iwdev->ldev->pcidev, 0) >> PAGE_SHIFT)
+		return -EINVAL;
+
 	if (vma->vm_pgoff == (db_addr_offset >> PAGE_SHIFT)) {
 		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	} else {
-- 
2.23.0

Re: [PATCH] RDMA/i40iw: Fix a mmap handler exploitation

[Jason Gunthorpe]

On Thu, Nov 19, 2020 at 05:35:23PM +0800, zhudi wrote:
> From: Di Zhu <zhudi21@huawei.com>
> 
> Notice that i40iw_mmap() is used as mmap for file
> /dev/infiniband/uverbs%d and these files have access mode
> with 0666 specified by uverbs_devnode() and vma->vm_pgoff
> is directly used to calculate pfn which is passed in
> remap_pfn_range function without any valid validation.
> 
> This would result in a malicious process being able to pass an arbitrary
> physical address to ‘mmap’ which would allow for access to all of
> kernel memory from user space and eventually gain root privileges.
> 
> So, we should check whether final calculated value of vm_pgoff is
> in range of specified pci resource before we use it to calculate
> pfn which is passed in remap_pfn_range
> 
> Signed-off-by: Di Zhu <zhudi21@huawei.com>

needs a  fixes line
and cc stable

> ---
>  drivers/infiniband/hw/i40iw/i40iw_verbs.c | 4 ++++
>  1 file changed, 4 insertions(+)

Wow. Yes, you are completely right, and this is a huge security flaw.

Shiraz, I think your company has some process for handling security
bugs of this magnitude, I suggest you get started.

I see the same bug exists in the irdma out of tree driver too.

> diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
> index 5ad381800f4d..7ec8f221eadb 100644
> --- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
> +++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
> @@ -185,6 +185,10 @@ static int i40iw_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
>  
>  	vma->vm_pgoff += db_addr_offset >> PAGE_SHIFT;
>  
> +	if (vma->vm_pgoff >
> +		pci_resource_len(ucontext->iwdev->ldev->pcidev, 0) >> PAGE_SHIFT)
> +		return -EINVAL;

I am willing to apply this if Shiraz confirms it is OK

However, it is not the right fix. Shiraz you need to send me a patch
to make proper use of the new mmap cookie framework.

I see in the userspace there are only 3 acceptable values for offset:

- 0
- i40iw_ucreate_qp_resp->push_idx + I40IW_BASE_PUSH_PAGE
- i40iw_ucreate_qp_resp->push_idx + I40IW_BASE_PUSH_PAGE + 1

So create mmap cookies for only those values and derive the pfn only
from entry after extracting it from the cookie. This should also be
blocking access to parts of the BAR the process is not allowed to
access.

EFA has a pretty easy to follow example for the API in __efa_mmap:

	rdma_entry = rdma_user_mmap_entry_get(&ucontext->ibucontext, vma);
[..]
	pfn = entry->address >> PAGE_SHIFT;
[..]
		err = rdma_user_mmap_io(&ucontext->ibucontext, vma, pfn,
					entry->rdma_entry.npages * PAGE_SIZE,
					pgprot_noncached(vma->vm_page_prot),
					rdma_entry);

efa_user_mmap_entry_insert() shows how to get the cookie you'd return
in push_idx, for compatability you'd have to make some adjustments
here, but there are APIs for that too, mlx5 has examples.

Jason

RE: [PATCH] RDMA/i40iw: Fix a mmap handler exploitation

[Saleem, Shiraz]

> Subject: Re: [PATCH] RDMA/i40iw: Fix a mmap handler exploitation
> 
> On Thu, Nov 19, 2020 at 05:35:23PM +0800, zhudi wrote:
> > From: Di Zhu <zhudi21@huawei.com>
> >
> > Notice that i40iw_mmap() is used as mmap for file
> > /dev/infiniband/uverbs%d and these files have access mode with 0666
> > specified by uverbs_devnode() and vma->vm_pgoff is directly used to
> > calculate pfn which is passed in remap_pfn_range function without any
> > valid validation.
> >
> > This would result in a malicious process being able to pass an
> > arbitrary physical address to ‘mmap’ which would allow for access to
> > all of kernel memory from user space and eventually gain root privileges.
> >
> > So, we should check whether final calculated value of vm_pgoff is in
> > range of specified pci resource before we use it to calculate pfn
> > which is passed in remap_pfn_range
> >
> > Signed-off-by: Di Zhu <zhudi21@huawei.com>
> 
> needs a  fixes line
> and cc stable
> 
> > ---
> >  drivers/infiniband/hw/i40iw/i40iw_verbs.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> 

[...]

> 
> I am willing to apply this if Shiraz confirms it is OK

I am ok with it. At least it reduces the severity of the issue ☹

> 
> However, it is not the right fix. Shiraz you need to send me a patch to make proper
> use of the new mmap cookie framework.
> 
> I see in the userspace there are only 3 acceptable values for offset:
> 
> - 0
> - i40iw_ucreate_qp_resp->push_idx + I40IW_BASE_PUSH_PAGE
> - i40iw_ucreate_qp_resp->push_idx + I40IW_BASE_PUSH_PAGE + 1

That’s right. But...see below

> 
> So create mmap cookies for only those values and derive the pfn only from entry
> after extracting it from the cookie. This should also be blocking access to parts of
> the BAR the process is not allowed to access.
> 
> EFA has a pretty easy to follow example for the API in __efa_mmap:
> 
> 	rdma_entry = rdma_user_mmap_entry_get(&ucontext->ibucontext, vma);
> [..]
> 	pfn = entry->address >> PAGE_SHIFT;
> [..]
> 		err = rdma_user_mmap_io(&ucontext->ibucontext, vma, pfn,
> 					entry->rdma_entry.npages * PAGE_SIZE,
> 					pgprot_noncached(vma->vm_page_prot),
> 					rdma_entry);
> 
> efa_user_mmap_entry_insert() shows how to get the cookie you'd return in
> push_idx, for compatability you'd have to make some adjustments here, but there
> are APIs for that too, mlx5 has examples.
> 

Well, the push feature is disabled by default and today there will be no push page mmap from
user-space since uresp.push_idx is an invalid one. Its disabled for good reason
as its not working as expected. There is an option to turn it on via module param but that
does not work as expected still resulting in an invalid uresp.push_idx passed to user-space
and no mmap.

So isn’t it better to just remove the push related code in the driver? which should also remove the manipulation on the vm_pgoff I believe.

I will review the mmap API and see how we can use it for DB mmap.

Shiraz

Re: [PATCH] RDMA/i40iw: Fix a mmap handler exploitation

[Jason Gunthorpe]

On Fri, Nov 20, 2020 at 11:56:36PM +0000, Saleem, Shiraz wrote:

> Well, the push feature is disabled by default and today there will
> be no push page mmap from user-space since uresp.push_idx is an
> invalid one. Its disabled for good reason as its not working as
> expected. There is an option to turn it on via module param but that
> does not work as expected still resulting in an invalid
> uresp.push_idx passed to user-space and no mmap.
> 
> So isn’t it better to just remove the push related code in the
> driver? which should also remove the manipulation on the vm_pgoff I
> believe.

Yes, delete all the push code, module param, etc. Set the invalid
push_idx, verify vm_pgoff == 0 and hardwire the pfn to be a single BAR
page.

Can you send a patch soon?

> I will review the mmap API and see how we can use it for DB mmap.

This would be something to use if you ever make it work again

Jason

RE: [PATCH] RDMA/i40iw: Fix a mmap handler exploitation

[Saleem, Shiraz]

> Subject: Re: [PATCH] RDMA/i40iw: Fix a mmap handler exploitation
> 
> On Fri, Nov 20, 2020 at 11:56:36PM +0000, Saleem, Shiraz wrote:
> 
> > Well, the push feature is disabled by default and today there will be
> > no push page mmap from user-space since uresp.push_idx is an invalid
> > one. Its disabled for good reason as its not working as expected.
> > There is an option to turn it on via module param but that does not
> > work as expected still resulting in an invalid uresp.push_idx passed
> > to user-space and no mmap.
> >
> > So isn’t it better to just remove the push related code in the driver?
> > which should also remove the manipulation on the vm_pgoff I believe.
> 
> Yes, delete all the push code, module param, etc. Set the invalid push_idx, verify
> vm_pgoff == 0 and hardwire the pfn to be a single BAR page.
> 
> Can you send a patch soon?

Yes.