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.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
drivers/usb/core/devio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 6972829ec6b5..56ea4143f8b1 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1591,7 +1591,6 @@ find_memory_area(struct usb_dev_state *ps, const struct usbdevfs_urb *uurb) { struct usb_memory *usbm = NULL, *iter; unsigned long flags;
- /* TODO [PCuABI] - capability checks for uaccess */ unsigned long uurb_start = user_ptr_addr(uurb->buffer);
spin_lock_irqsave(&ps->lock, flags); @@ -1773,6 +1772,10 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb ret = -EFAULT; goto error; }
- if (unlikely(!check_user_ptr_rw(uurb->buffer, uurb->buffer_length))) {
My main point in v2 was that this should move here to check the appropriate permissions - we must be careful not to require RW permissions when just R or W is actually necessary. The is_in variable seems to indicate the direction of the transfer.
Also, it would make more sense for that check to be together with the access_ok() check above, because it is preceded with a check on uurb->buffer_length. I'm not completely convinced that it is valid for it to be 0 in the first place, but either way we should be consistent.
Kevin
ret = -EFAULT;goto error;- } as = alloc_async(number_of_packets); if (!as) { ret = -ENOMEM;