On Fri, Sep 28, 2018 at 7:50 PM, Catalin Marinas catalin.marinas@arm.com wrote:
On Mon, Sep 17, 2018 at 07:01:00PM +0200, Andrey Konovalov wrote:
Looking at patch #8 ("usb, arm64: untag user addresses in devio") in this series, it seems that that devio ioctl actually accepts a pointer into a vma, so we shouldn't actually be untagging its argument and the patch needs to be dropped.
You are right, the pointer seems to have originated from the kernel as already untagged (mmap() on the driver), so we would expect the user to pass it back an untagged pointer.
OK, dropped this patch in v7.
As for case 1, the places where pointers are compared with TASK_SIZE and others can be found with grep. Maybe it makes sense to introduce some kind of routine like is_user_pointer() that handles tagged pointers and refactor the existing code to use it? And maybe add a rule to checkpatch.pl that forbids the direct usage of TASK_SIZE and others.
So I think detecting direct comparisons with TASK_SIZE and others would more useful than finding __user pointer casts (it seems that the latter requires a lot of annotations to be fixed/added), and I should just drop this patch with annotations.
I think point (1) is not too bad, usually found with grep.
As I've said in my previous reply, I kind of came to the same conclusion that searching __user pointer casts to long may not actually scale. If we could add an __untagged annotation to ulong where it matters (e.g. find_vma()), we could identify a ulong (default tagged) and annotate some of those.
However, this analysis on __user * casting was useful even if we don't end up using it. If we come up with a clearer definition of the ABI (which syscalls accept tagged pointers), we may conclude that the only places where untagging matters are a few find_vma() calls in the arch and mm code and can ignore the rest.
So what exactly should I do now?
For now I've posted v7 with the sparse annotation patch dropped (to have the most up-do-date version posted).