block layer zero-copy: missing access_ok() check

Quoted 6 lines · * Linus Torvalds ([email protected]) wrote:

It's a bit subtle, but at least the x86 get-user-pages does actually check access_ok() implicitly. It's just that it does so using the bits in the page table, and does the page table lookup as a "user access". So it checks the page tables themselves, not the user limit.

Quoted 7 lines

Which is fine, because that's what the *hardware* does. So if the page tables make something readable to users, then they are readable by definition.

So get_user_pages_fast() doesn't need access_ok() before it, and the naming isn't actually confusing. And I'm sure we knew this at some point.

Ah, I see! so my guess is that it is expected that "gup_*" functions implicitly check that they are getting user pages. If we look at this through fresh eyes, across all architectures:

* x86: looks OK: gup* checks with _PAGE_USER flag. The slow path that goes through __get_user_pages() seem to rely on follow_page_mask() and then pgd_bad() as well as pud_bad() to check the _PAGE_USER flag.

* mips: access_ok missing in get_user_pages_fast,
-> I don't see any explicit mention of "USER" pages flags within the
gup functions.

* powerpc: access_ok is there, everything is fine,

* s390: access_ok missing in both __get_user_pages_fast and get_user_pages_fast.
-> I don't see clear indication of USER pages being flagged.

* sh: access_ok missing in get_user_pages_fast, -> OK, gup_* functions are checking a _PAGE_USER flag.

* sparc: access_ok missing in get_user_pages_fast, -> no indication of any _PAGE_USER flag.

* generic: mm/util.c:get_user_pages_fast() ends up calling mm/memory.c:get_user_pages() and then __get_user_pages(), which are also used as slow-path for all architectures above:

-> from my understanding, through follow_page_mask() pgd_bad() and
pud_bad() are checking _PAGE_USER flags (when they exist). Unfortunately, the following grep is slightly worrying:

Adding linux-arch. Guys, can you check your architectures?

Also, make sure to check huge-pages if they are separate. Basically, if you have code like this:

                if (!pte_present(pte) ||
                    pte_special(pte) || (write && !pte_write(pte))) {
                        pte_unmap(ptep);
                        return 0;
                }

it's probably buggy. It's not sufficient to just check write permissions, you do need to check user permissions too.

Powerpc,x86 and sh seem to get it right by virtue of checking rthe user bit. s390 checks against TASK_SIZE.

MIPS does seem buggy. Sparc I don't know the meaning of the bits for. And powerpc does have several variants, so while the main one looks fine, I didn't look at the other ones.

Linus

Quoted 46 lines · Mathieu Desnoyers

Quoted 6 lines · * Linus Torvalds ([email protected]) wrote:

It's a bit subtle, but at least the x86 get-user-pages does actually check access_ok() implicitly. It's just that it does so using the bits in the page table, and does the page table lookup as a "user access". So it checks the page tables themselves, not the user limit.

Quoted 7 lines

Which is fine, because that's what the *hardware* does. So if the page tables make something readable to users, then they are readable by definition.

So get_user_pages_fast() doesn't need access_ok() before it, and the naming isn't actually confusing. And I'm sure we knew this at some point.

Ah, I see! so my guess is that it is expected that "gup_*" functions implicitly check that they are getting user pages. If we look at this through fresh eyes, across all architectures:

* x86: looks OK: gup* checks with _PAGE_USER flag. The slow path that goes through __get_user_pages() seem to rely on follow_page_mask() and then pgd_bad() as well as pud_bad() to check the _PAGE_USER flag.

* mips: access_ok missing in get_user_pages_fast,
-> I don't see any explicit mention of "USER" pages flags within the
gup functions.

* powerpc: access_ok is there, everything is fine,

* s390: access_ok missing in both __get_user_pages_fast and get_user_pages_fast.
-> I don't see clear indication of USER pages being flagged.

* sh: access_ok missing in get_user_pages_fast, -> OK, gup_* functions are checking a _PAGE_USER flag.

* sparc: access_ok missing in get_user_pages_fast, -> no indication of any _PAGE_USER flag.

* generic: mm/util.c:get_user_pages_fast() ends up calling mm/memory.c:get_user_pages() and then __get_user_pages(), which are also used as slow-path for all architectures above:

-> from my understanding, through follow_page_mask() pgd_bad() and
pud_bad() are checking _PAGE_USER flags (when they exist). Unfortunately, the following grep is slightly worrying:

In addition to get_user_pages_fast() issues, I see that there are many direct callers of get_user_pages() that seem to assume that access checks are performed within this function.

AFAIU, on architectures that
have a _PAGE_USER flag, this check is performed internally by pgd_bad() and pud_bad(), but what happens to all the others ?

get_user_pages() does check permissions. It looks up the vma and checks them there, which is much more than access_ok() ever does.

Quoted 3 lines · Linus Torvalds

get_user_pages() does check permissions. It looks up the vma and checks them there, which is much more than access_ok() ever does.

To clarify: the fact that a vma exists at all already means "it's a user mapping" (with the special gate area being a secondary user mapping). So you don't need any kind of explicit user check.

Adding linux-arch. Guys, can you check your architectures?

Also, make sure to check huge-pages if they are separate. Basically, if you have code like this:

                if (!pte_present(pte) ||
                    pte_special(pte) || (write && !pte_write(pte))) {
                        pte_unmap(ptep);
                        return 0;
                }

it's probably buggy. It's not sufficient to just check write permissions, you do need to check user permissions too.

Powerpc,x86 and sh seem to get it right by virtue of checking rthe user bit. s390 checks against TASK_SIZE.

MIPS does seem buggy. Sparc I don't know the meaning of the bits for. And powerpc does have several variants, so while the main one looks fine, I didn't look at the other ones.

Adding linux-arch. Guys, can you check your architectures?

Quoted 2 lines

* sparc: access_ok missing in get_user_pages_fast, -> no indication of any _PAGE_USER flag.