From: Ira Weiny <[email protected]>
The kernel pages used by shm_get_kernel_pages() are allocated using
GFP_KERNEL through the following call stack:
trusted_instantiate()
trusted_payload_alloc() -> GFP_KERNEL
<trusted key op>
tee_shm_register_kernel_buf()
register_shm_helper()
shm_get_kernel_pages()
Where <trusted key op> is one of:
trusted_key_unseal()
trusted_key_get_random()
trusted_key_seal()
Because the pages can't be from highmem get_kernel_pages() boils down to
a get_page() call.
Remove the get_kernel_pages() call and open code the get_page().
In case a kmap page does slip through warn on once for a kmap address.
Cc: Jens Wiklander <[email protected]>
Cc: Al Viro <[email protected]>
Cc: "Fabio M. De Francesco" <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Linus Torvalds <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
drivers/tee/tee_shm.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 527a6eabc03e..45e6ff1a452e 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -11,6 +11,7 @@
#include <linux/tee_drv.h>
#include <linux/uaccess.h>
#include <linux/uio.h>
+#include <linux/highmem.h>
#include "tee_private.h"
static void shm_put_kernel_pages(struct page **pages, size_t page_count)
@@ -26,9 +27,9 @@ static int shm_get_kernel_pages(unsigned long start, size_t page_count,
{
struct kvec *kiov;
size_t n;
- int rc;
- if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
+ if (WARN_ON_ONCE(is_vmalloc_addr((void *)start) ||
+ is_kmap_addr((void *)start)))
return -EINVAL;
kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
@@ -38,12 +39,12 @@ static int shm_get_kernel_pages(unsigned long start, size_t page_count,
for (n = 0; n < page_count; n++) {
kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
kiov[n].iov_len = PAGE_SIZE;
+ pages[n] = virt_to_page(kiov[n].iov_base);
+ get_page(pages[n]);
}
-
- rc = get_kernel_pages(kiov, page_count, 0, pages);
kfree(kiov);
- return rc;
+ return page_count;
}
static void release_registered_pages(struct tee_shm *shm)
--
2.37.2
On Sat, Oct 01, 2022 at 05:23:25PM -0700, [email protected] wrote:
> kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> @@ -38,12 +39,12 @@ static int shm_get_kernel_pages(unsigned long start, size_t page_count,
> for (n = 0; n < page_count; n++) {
> kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> kiov[n].iov_len = PAGE_SIZE;
> + pages[n] = virt_to_page(kiov[n].iov_base);
> + get_page(pages[n]);
> }
> -
> - rc = get_kernel_pages(kiov, page_count, 0, pages);
> kfree(kiov);
IDGI. The only thing in kiov[...] you are every reading is
->iov_base. And you fetch it once, right after the assignment.
Why bother with allocating the array at all?
pages[n] = virt_to_page((void *)start + n * PAGE_SIZE);
would do just as well, not to mention the fact that since you reject
vmalloc and kmap, you might simply do
page = virt_to_page(start);
for (int n = 0; n < page_count; n++)
get_page(pages[n] = page + n);
instead...
On Sun, Oct 02, 2022 at 01:46:41AM +0100, Al Viro wrote:
> On Sat, Oct 01, 2022 at 05:23:25PM -0700, [email protected] wrote:
>
> > kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> > @@ -38,12 +39,12 @@ static int shm_get_kernel_pages(unsigned long start, size_t page_count,
> > for (n = 0; n < page_count; n++) {
> > kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> > kiov[n].iov_len = PAGE_SIZE;
> > + pages[n] = virt_to_page(kiov[n].iov_base);
> > + get_page(pages[n]);
> > }
> > -
> > - rc = get_kernel_pages(kiov, page_count, 0, pages);
> > kfree(kiov);
>
> IDGI. The only thing in kiov[...] you are every reading is
> ->iov_base. And you fetch it once, right after the assignment.
:-( Good point. Thanks for catching that. I was too focused on just
replacing get_kernel_pages() with get_page() and I should have refactored
more.
>
> Why bother with allocating the array at all?
> pages[n] = virt_to_page((void *)start + n * PAGE_SIZE);
> would do just as well, not to mention the fact that since you reject
> vmalloc and kmap, you might simply do
>
> page = virt_to_page(start);
> for (int n = 0; n < page_count; n++)
> get_page(pages[n] = page + n);
I think I'd avoid the assignment in the parameter as I would miss that if I
came back and looked at this code later.
I'll get rid of the kiov in v2.
Sorry for not cleaning it up more and thanks for the review!
Ira
On Sun, Oct 02, 2022 at 01:46:41AM +0100, Al Viro wrote:
> IDGI. The only thing in kiov[...] you are every reading is
> ->iov_base. And you fetch it once, right after the assignment.
... and that's exactly what I pointed out on the last version of the
patch ...
On Mon, Oct 03, 2022 at 09:17:18AM +0200, Christoph Hellwig wrote:
> On Sun, Oct 02, 2022 at 01:46:41AM +0100, Al Viro wrote:
> > IDGI. The only thing in kiov[...] you are every reading is
> > ->iov_base. And you fetch it once, right after the assignment.
>
> ... and that's exactly what I pointed out on the last version of the
> patch ...
Apologies I guess I missed it.
Ira