[PATCH] RDMA/siw: fix pointer cast warning

[PATCH] RDMA/siw: fix pointer cast warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
 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

RE: [PATCH] RDMA/siw: fix pointer cast warning

[Bernard Metzler]

> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: Thursday, 15 December 2022 18:04
> To: Bernard Metzler <BMT@zurich.ibm.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> Leon Romanovsky <leon@kernel.org>; Linus Walleij <linus.walleij@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>; linux-rdma@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXTERNAL] [PATCH] RDMA/siw: fix pointer cast warning
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <arnd@arndb.de>
> ---
>  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 <bmt@zurich.ibm.com>

RE: [PATCH] RDMA/siw: fix pointer cast warning

[David Laight]

From: Arnd Bergmann
> Sent: 15 December 2022 17:04
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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)

Re: [PATCH] RDMA/siw: fix pointer cast warning

[Linus Walleij]

On Thu, Dec 15, 2022 at 11:20 PM David Laight <David.Laight@aculab.com> wrote:

> From: Arnd Bergmann
> > Sent: 15 December 2022 17:04
> >
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > 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

RE: Re: [PATCH] RDMA/siw: fix pointer cast warning

[Bernard Metzler]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Friday, 16 December 2022 08:47
> To: David Laight <David.Laight@aculab.com>
> Cc: Arnd Bergmann <arnd@kernel.org>; Bernard Metzler <BMT@zurich.ibm.com>;
> Jason Gunthorpe <jgg@ziepe.ca>; Leon Romanovsky <leon@kernel.org>; Arnd
> Bergmann <arnd@arndb.de>; linux-rdma@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: fix pointer cast warning
> 
> On Thu, Dec 15, 2022 at 11:20 PM David Laight <David.Laight@aculab.com>
> wrote:
> 
> > From: Arnd Bergmann
> > > Sent: 15 December 2022 17:04
> > >
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > 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

RE: Re: [PATCH] RDMA/siw: fix pointer cast warning

[David Laight]

From: Bernard Metzler
> Sent: 16 December 2022 10:01
> 
> > -----Original Message-----
> > From: Linus Walleij <linus.walleij@linaro.org>
> > Sent: Friday, 16 December 2022 08:47
> > To: David Laight <David.Laight@aculab.com>
> > Cc: Arnd Bergmann <arnd@kernel.org>; Bernard Metzler <BMT@zurich.ibm.com>;
> > Jason Gunthorpe <jgg@ziepe.ca>; Leon Romanovsky <leon@kernel.org>; Arnd
> > Bergmann <arnd@arndb.de>; linux-rdma@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: fix pointer cast warning
> >
> > On Thu, Dec 15, 2022 at 11:20 PM David Laight <David.Laight@aculab.com>
> > wrote:
> >
> > > From: Arnd Bergmann
> > > > Sent: 15 December 2022 17:04
> > > >
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > 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)

RE: Re: [PATCH] RDMA/siw: fix pointer cast warning

[Bernard Metzler]

> -----Original Message-----
> From: David Laight <David.Laight@ACULAB.COM>
> Sent: Friday, 16 December 2022 11:23
> To: Bernard Metzler <BMT@zurich.ibm.com>; Linus Walleij
> <linus.walleij@linaro.org>
> Cc: Arnd Bergmann <arnd@kernel.org>; Jason Gunthorpe <jgg@ziepe.ca>; Leon
> Romanovsky <leon@kernel.org>; Arnd Bergmann <arnd@arndb.de>; linux-
> rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> 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 <linus.walleij@linaro.org>
> > > Sent: Friday, 16 December 2022 08:47
> > > To: David Laight <David.Laight@aculab.com>
> > > Cc: Arnd Bergmann <arnd@kernel.org>; Bernard Metzler
> <BMT@zurich.ibm.com>;
> > > Jason Gunthorpe <jgg@ziepe.ca>; Leon Romanovsky <leon@kernel.org>; Arnd
> > > Bergmann <arnd@arndb.de>; linux-rdma@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: fix pointer cast warning
> > >
> > > On Thu, Dec 15, 2022 at 11:20 PM David Laight <David.Laight@aculab.com>
> > > wrote:
> > >
> > > > From: Arnd Bergmann
> > > > > Sent: 15 December 2022 17:04
> > > > >
> > > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > >
> > > > > 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)

Re: [PATCH] RDMA/siw: fix pointer cast warning

[Linus Walleij]

On Thu, Dec 15, 2022 at 6:03 PM Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
>
> 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 <arnd@arndb.de>

This driver is a big confusion for me too.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij