| Message ID | 20221215170347.2612403-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5244ca88671a1981ceec09c5c8809f003e6a62aa |
| Headers | show |
| Series | RDMA/siw: fix pointer cast warning | expand |
> -----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>
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)
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
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
> -----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
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)
> -----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)
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; }