From: Arnd Bergmann <[email protected]>
The previous build fix left a remaining issue in configurations
with 64-bit dma_addr_t on 32-bit architectures:
drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_get_pblpage':
drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
32 | return virt_to_page((void *)paddr);
| ^
Use the same double cast here that the driver uses elsewhere
to convert between dma_addr_t and void*.
It took me a while to figure out why this driver does it
like this, as there is no hardware access and it just stores
kernel pointers in place of device addresses when communicating
with the rdma core and with user space.
Fixes: 0d1b756acf60 ("RDMA/siw: Pass a pointer to virt_to_page()")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/infiniband/sw/siw/siw_qp_tx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index 7d47b521070b..05052b49107f 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -29,7 +29,7 @@ static struct page *siw_get_pblpage(struct siw_mem *mem, u64 addr, int *idx)
dma_addr_t paddr = siw_pbl_get_buffer(pbl, offset, NULL, idx);
if (paddr)
- return virt_to_page((void *)paddr);
+ return virt_to_page((void *)(uintptr_t)paddr);
return NULL;
}
--
2.35.1
> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: Thursday, 15 December 2022 18:04
> To: Bernard Metzler <[email protected]>; Jason Gunthorpe <[email protected]>;
> Leon Romanovsky <[email protected]>; Linus Walleij <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: [EXTERNAL] [PATCH] RDMA/siw: fix pointer cast warning
>
> From: Arnd Bergmann <[email protected]>
>
> The previous build fix left a remaining issue in configurations
> with 64-bit dma_addr_t on 32-bit architectures:
>
> drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_get_pblpage':
> drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer from
> integer of different size [-Werror=int-to-pointer-cast]
> 32 | return virt_to_page((void *)paddr);
> | ^
>
> Use the same double cast here that the driver uses elsewhere
> to convert between dma_addr_t and void*.
>
> It took me a while to figure out why this driver does it
> like this, as there is no hardware access and it just stores
> kernel pointers in place of device addresses when communicating
> with the rdma core and with user space.
>
> Fixes: 0d1b756acf60 ("RDMA/siw: Pass a pointer to virt_to_page()")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/infiniband/sw/siw/siw_qp_tx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
> b/drivers/infiniband/sw/siw/siw_qp_tx.c
> index 7d47b521070b..05052b49107f 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_tx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> @@ -29,7 +29,7 @@ static struct page *siw_get_pblpage(struct siw_mem *mem,
> u64 addr, int *idx)
> dma_addr_t paddr = siw_pbl_get_buffer(pbl, offset, NULL, idx);
>
> if (paddr)
> - return virt_to_page((void *)paddr);
> + return virt_to_page((void *)(uintptr_t)paddr);
>
> return NULL;
> }
Thanks Arnd, makes complete sense.
Acked-by: Bernard Metzler <[email protected]>
From: Arnd Bergmann
> Sent: 15 December 2022 17:04
>
> From: Arnd Bergmann <[email protected]>
>
> The previous build fix left a remaining issue in configurations
> with 64-bit dma_addr_t on 32-bit architectures:
>
> drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_get_pblpage':
> drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer from integer of different size [-
> Werror=int-to-pointer-cast]
> 32 | return virt_to_page((void *)paddr);
> | ^
>
> Use the same double cast here that the driver uses elsewhere
> to convert between dma_addr_t and void*.
>
> It took me a while to figure out why this driver does it
> like this, as there is no hardware access and it just stores
> kernel pointers in place of device addresses when communicating
> with the rdma core and with user space.
I hope that doesn't mean it is relying on user space only
giving it back valid values?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, Dec 15, 2022 at 6:03 PM Arnd Bergmann <[email protected]> wrote:
> From: Arnd Bergmann <[email protected]>
>
> The previous build fix left a remaining issue in configurations
> with 64-bit dma_addr_t on 32-bit architectures:
>
> drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_get_pblpage':
> drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
> 32 | return virt_to_page((void *)paddr);
> | ^
>
> Use the same double cast here that the driver uses elsewhere
> to convert between dma_addr_t and void*.
>
> It took me a while to figure out why this driver does it
> like this, as there is no hardware access and it just stores
> kernel pointers in place of device addresses when communicating
> with the rdma core and with user space.
>
> Fixes: 0d1b756acf60 ("RDMA/siw: Pass a pointer to virt_to_page()")
> Signed-off-by: Arnd Bergmann <[email protected]>
This driver is a big confusion for me too.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Dec 15, 2022 at 11:20 PM David Laight <[email protected]> wrote:
> From: Arnd Bergmann
> > Sent: 15 December 2022 17:04
> >
> > From: Arnd Bergmann <[email protected]>
> >
> > The previous build fix left a remaining issue in configurations
> > with 64-bit dma_addr_t on 32-bit architectures:
> >
> > drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_get_pblpage':
> > drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer from integer of different size [-
> > Werror=int-to-pointer-cast]
> > 32 | return virt_to_page((void *)paddr);
> > | ^
> >
> > Use the same double cast here that the driver uses elsewhere
> > to convert between dma_addr_t and void*.
> >
> > It took me a while to figure out why this driver does it
> > like this, as there is no hardware access and it just stores
> > kernel pointers in place of device addresses when communicating
> > with the rdma core and with user space.
>
> I hope that doesn't mean it is relying on user space only
> giving it back valid values?
It looks to me like this driver totally trusts userspace.
Yours,
Linus Walleij
> -----Original Message-----
> From: Linus Walleij <[email protected]>
> Sent: Friday, 16 December 2022 08:47
> To: David Laight <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; Bernard Metzler <[email protected]>;
> Jason Gunthorpe <[email protected]>; Leon Romanovsky <[email protected]>; Arnd
> Bergmann <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: fix pointer cast warning
>
> On Thu, Dec 15, 2022 at 11:20 PM David Laight <[email protected]>
> wrote:
>
> > From: Arnd Bergmann
> > > Sent: 15 December 2022 17:04
> > >
> > > From: Arnd Bergmann <[email protected]>
> > >
> > > The previous build fix left a remaining issue in configurations
> > > with 64-bit dma_addr_t on 32-bit architectures:
> > >
> > > drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_get_pblpage':
> > > drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer
> from integer of different size [-
> > > Werror=int-to-pointer-cast]
> > > 32 | return virt_to_page((void *)paddr);
> > > | ^
> > >
> > > Use the same double cast here that the driver uses elsewhere
> > > to convert between dma_addr_t and void*.
> > >
> > > It took me a while to figure out why this driver does it
> > > like this, as there is no hardware access and it just stores
> > > kernel pointers in place of device addresses when communicating
> > > with the rdma core and with user space.
> >
> > I hope that doesn't mean it is relying on user space only
> > giving it back valid values?
>
> It looks to me like this driver totally trusts userspace.
>
Shame on me. Yes, somehow, an access_ok((void __user *)start, len)
is missing! Let me fix that when I am back at my desk. Seems it needs
immediate action.
Many thanks for pointing that out!
Bernard.
> Yours,
> Linus Walleij
From: Bernard Metzler
> Sent: 16 December 2022 10:01
>
> > -----Original Message-----
> > From: Linus Walleij <[email protected]>
> > Sent: Friday, 16 December 2022 08:47
> > To: David Laight <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>; Bernard Metzler <[email protected]>;
> > Jason Gunthorpe <[email protected]>; Leon Romanovsky <[email protected]>; Arnd
> > Bergmann <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: fix pointer cast warning
> >
> > On Thu, Dec 15, 2022 at 11:20 PM David Laight <[email protected]>
> > wrote:
> >
> > > From: Arnd Bergmann
> > > > Sent: 15 December 2022 17:04
> > > >
> > > > From: Arnd Bergmann <[email protected]>
> > > >
> > > > The previous build fix left a remaining issue in configurations
> > > > with 64-bit dma_addr_t on 32-bit architectures:
> > > >
> > > > drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_get_pblpage':
> > > > drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer
> > from integer of different size [-
> > > > Werror=int-to-pointer-cast]
> > > > 32 | return virt_to_page((void *)paddr);
> > > > | ^
> > > >
> > > > Use the same double cast here that the driver uses elsewhere
> > > > to convert between dma_addr_t and void*.
> > > >
> > > > It took me a while to figure out why this driver does it
> > > > like this, as there is no hardware access and it just stores
> > > > kernel pointers in place of device addresses when communicating
> > > > with the rdma core and with user space.
> > >
> > > I hope that doesn't mean it is relying on user space only
> > > giving it back valid values?
> >
> > It looks to me like this driver totally trusts userspace.
> >
>
> Shame on me. Yes, somehow, an access_ok((void __user *)start, len)
> is missing! Let me fix that when I am back at my desk. Seems it needs
> immediate action.
That wasn't the sort of issue I was thinking about.
I was worried that it was putting the addresses of kernel memory
into buffers written to userspace and then later reading the
addresses back from userspace and accessing them.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
> -----Original Message-----
> From: David Laight <[email protected]>
> Sent: Friday, 16 December 2022 11:23
> To: Bernard Metzler <[email protected]>; Linus Walleij
> <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; Jason Gunthorpe <[email protected]>; Leon
> Romanovsky <[email protected]>; Arnd Bergmann <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: [EXTERNAL] RE: Re: [PATCH] RDMA/siw: fix pointer cast warning
>
> From: Bernard Metzler
> > Sent: 16 December 2022 10:01
> >
> > > -----Original Message-----
> > > From: Linus Walleij <[email protected]>
> > > Sent: Friday, 16 December 2022 08:47
> > > To: David Laight <[email protected]>
> > > Cc: Arnd Bergmann <[email protected]>; Bernard Metzler
> <[email protected]>;
> > > Jason Gunthorpe <[email protected]>; Leon Romanovsky <[email protected]>; Arnd
> > > Bergmann <[email protected]>; [email protected]; linux-
> > > [email protected]
> > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: fix pointer cast warning
> > >
> > > On Thu, Dec 15, 2022 at 11:20 PM David Laight <[email protected]>
> > > wrote:
> > >
> > > > From: Arnd Bergmann
> > > > > Sent: 15 December 2022 17:04
> > > > >
> > > > > From: Arnd Bergmann <[email protected]>
> > > > >
> > > > > The previous build fix left a remaining issue in configurations
> > > > > with 64-bit dma_addr_t on 32-bit architectures:
> > > > >
> > > > > drivers/infiniband/sw/siw/siw_qp_tx.c: In function
> 'siw_get_pblpage':
> > > > > drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: error: cast to pointer
> > > from integer of different size [-
> > > > > Werror=int-to-pointer-cast]
> > > > > 32 | return virt_to_page((void *)paddr);
> > > > > | ^
> > > > >
> > > > > Use the same double cast here that the driver uses elsewhere
> > > > > to convert between dma_addr_t and void*.
> > > > >
> > > > > It took me a while to figure out why this driver does it
> > > > > like this, as there is no hardware access and it just stores
> > > > > kernel pointers in place of device addresses when communicating
> > > > > with the rdma core and with user space.
> > > >
> > > > I hope that doesn't mean it is relying on user space only
> > > > giving it back valid values?
> > >
> > > It looks to me like this driver totally trusts userspace.
> > >
> >
> > Shame on me. Yes, somehow, an access_ok((void __user *)start, len)
> > is missing! Let me fix that when I am back at my desk. Seems it needs
> > immediate action.
>
> That wasn't the sort of issue I was thinking about.
> I was worried that it was putting the addresses of kernel memory
> into buffers written to userspace and then later reading the
> addresses back from userspace and accessing them.
>
Oh, no, that is not the case. The address mapping is not accessible
from userspace. It is local to the kernel driver only to translate
user virtual addresses to kernel pages during transmit/receive of
application data. The user only knows about its own VA's it uses
in its work requests, and during buffer registration.
BUT, you pointed me to something bad. Checking the users permission
to register requested memory with the driver is definitely missing.
I was under the wrong impression it would be checked by used
pin_user_pages(), but that is not the case. pin_user_pages_fast()
does that check, other drivers using it, and it looks like a good fix.
Other rdma drivers, like infiniband/hw/usnic, wich also use
pin_user_pages(), may suffer from the same problem. There is no
checking of user permissions for the memory being registered
from user land.
Thanks very much,
Bernard.
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1
> 1PT, UK
> Registration No: 1397386 (Wales)