On 09/08/2023 12:07, Luca Vizzarro wrote:
Whenever a GUP call is made, a user address in the form of a 64-bit integer is used to lookup its corresponding page. When working in PCuABI, this means that the metadata of the user capability gets discarded, hence any access made by the GUP is not checked in hardware.
This commit introduces explicit capability checks whenever a call to the current mm through the GUP functions is made.
The comment I made on patch 2 in v2 applied to all patches: the commit message should explain what each specific patch does (though it's ok to repeat one paragraph across the series if it helps).
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
lib/iov_iter.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 3b082f598162..e9c6f5b17768 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1538,12 +1538,15 @@ static ssize_t iter_xarray_get_pages(struct iov_iter *i, /* must be done on non-empty ITER_UBUF or ITER_IOVEC one */ static unsigned long first_iovec_segment(const struct iov_iter *i, size_t *size) {
- bool is_source = iov_iter_rw(i) == ITER_SOURCE;
- void __user *seg; size_t skip; long k;
- if (iter_is_ubuf(i))
/* TODO [PCuABI] - capability checks for uaccess */return user_ptr_addr(i->ubuf) + i->iov_offset;
- if (iter_is_ubuf(i)) {
seg = i->ubuf + i->iov_offset;goto ptr_check;- }
for (k = 0, skip = i->iov_offset; k < i->nr_segs; k++, skip = 0) { const struct iovec *iov = iter_iov(i) + k; @@ -1553,9 +1556,19 @@ static unsigned long first_iovec_segment(const struct iov_iter *i, size_t *size) continue; if (*size > len) *size = len;
return user_ptr_addr(iov->iov_base) + skip;
seg = iov->iov_base + skip;goto ptr_check;+ptr_check:
- if (unlikely(is_source && !check_user_ptr_read(seg, *size)))
return -EFAULT;- if (unlikely(!is_source && !check_user_ptr_write(seg, *size)))
return -EFAULT;
Nit: could combine the two ifs into one (convenient when changing the function to return a user pointer - see comment below - as then you only need to write ERR_USER_PTR(-EFAULT) once).
- return user_ptr_addr(seg);
} /* must be done on non-empty ITER_BVEC one */ @@ -1600,6 +1613,9 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i, gup_flags |= FOLL_NOFAULT; addr = first_iovec_segment(i, &maxsize);
if (IS_ERR_VALUE(addr))return addr;- *start = addr % PAGE_SIZE; addr &= PAGE_MASK; n = want_pages_array(pages, maxsize, *start, maxpages);
@@ -2317,6 +2333,8 @@ static ssize_t iov_iter_extract_user_pages(struct iov_iter *i,
In v2 I mentioned the risk that first_iovec_segment() might get used in more places, and it looks like this is exactly what happened. It's a good thing you found out, and of course it means the checks need to stay in first_iovec_segment(). However, the fact that each caller needs to check that the returned value is valid remains a concern: once this patch is merged, we may not spot new callers when rebasing, meaning error checking will be missing. My suggestion in v2 was to make first_iovec_segment() return the actual user pointer, so that we have to change all callers (it will fail to build otherwise), and I think it still makes sense.
Kevin
gup_flags |= FOLL_NOFAULT;addr = first_iovec_segment(i, &maxsize);
- if (IS_ERR_VALUE(addr))
return addr;