Re: get_user_pages() and EXEC_ONLY mapping.

Re: get_user_pages() and EXEC_ONLY mapping.

[Jason Gunthorpe]

On Fri, Nov 10, 2023 at 08:19:23PM +0530, Aneesh Kumar K.V wrote:
> 
> Hello,
> 
> Some architectures can now support EXEC_ONLY mappings and I am wondering
> what get_user_pages() on those addresses should return. 

-EPERM

> Earlier PROT_EXEC implied PROT_READ and pte_access_permitted()
> returned true for that. But arm64 does have this explicit comment
> that says
> 
>  /*
>  * p??_access_permitted() is true for valid user mappings (PTE_USER
>  * bit set, subject to the write permission check). For execute-only
>  * mappings, like PROT_EXEC with EPAN (both PTE_USER and PTE_UXN bits
>  * not set) must return false. PROT_NONE mappings do not have the
>  * PTE_VALID bit set.
>  */
> 
> Is that correct? We should be able to get struct page for PROT_EXEC
> mappings?

If the memory is unreadable then providing a back door through
O_DIRECT and everthing else to read it sounds wrong to me.

If there is some case where a get_user_pages caller is exec-only
compatible then a new FOLL_EXEC flag to permit it would make sense.

Jason

Re: get_user_pages() and EXEC_ONLY mapping.

[Jason Gunthorpe]

On Fri, Nov 10, 2023 at 08:27:19PM +0530, Aneesh Kumar K V wrote:
> On 11/10/23 8:23 PM, Jason Gunthorpe wrote:
> > On Fri, Nov 10, 2023 at 08:19:23PM +0530, Aneesh Kumar K.V wrote:
> >>
> >> Hello,
> >>
> >> Some architectures can now support EXEC_ONLY mappings and I am wondering
> >> what get_user_pages() on those addresses should return. 
> > 
> > -EPERM
> > 
> >> Earlier PROT_EXEC implied PROT_READ and pte_access_permitted()
> >> returned true for that. But arm64 does have this explicit comment
> >> that says
> >>
> >>  /*
> >>  * p??_access_permitted() is true for valid user mappings (PTE_USER
> >>  * bit set, subject to the write permission check). For execute-only
> >>  * mappings, like PROT_EXEC with EPAN (both PTE_USER and PTE_UXN bits
> >>  * not set) must return false. PROT_NONE mappings do not have the
> >>  * PTE_VALID bit set.
> >>  */
> >>
> >> Is that correct? We should be able to get struct page for PROT_EXEC
> >> mappings?
> > 
> > If the memory is unreadable then providing a back door through
> > O_DIRECT and everthing else to read it sounds wrong to me.
> > 
> > If there is some case where a get_user_pages caller is exec-only
> > compatible then a new FOLL_EXEC flag to permit it would make sense.
> > 
> 
> I was expecting pin_user_pages() to return -EPERM and get_user_pages()
> return struct page. This was based on Documentation/core-api/pin_user_pages.rst  

Not unconditionally but you could argue that FOLL_GET should
succeed. It depends how much do you care about absolute security of
unreadable memory vs compatability.

> "Another way of thinking about these flags is as a progression of restrictions:
> FOLL_GET is for struct page manipulation, without affecting the data that the
> struct page refers to. FOLL_PIN is a *replacement* for FOLL_GET, and is for
> short term pins on pages whose data *will* get accessed. "

This was sort of aspirational - have we got rid of all the FOLL_GET
users that are touching the data? Looks like no from a quick check..

Jason

Re: get_user_pages() and EXEC_ONLY mapping.

[Catalin Marinas]

On Fri, Nov 10, 2023 at 08:19:23PM +0530, Aneesh Kumar K.V wrote:
> Some architectures can now support EXEC_ONLY mappings and I am wondering
> what get_user_pages() on those addresses should return. Earlier
> PROT_EXEC implied PROT_READ and pte_access_permitted() returned true for
> that. But arm64 does have this explicit comment that says
> 
>  /*
>  * p??_access_permitted() is true for valid user mappings (PTE_USER
>  * bit set, subject to the write permission check). For execute-only
>  * mappings, like PROT_EXEC with EPAN (both PTE_USER and PTE_UXN bits
>  * not set) must return false. PROT_NONE mappings do not have the
>  * PTE_VALID bit set.
>  */
> 
> Is that correct? We should be able to get struct page for PROT_EXEC
> mappings?

I don't remember why we ended up with this briefly looking at the code,
pte_access_permitted() is only used on the fast GUP path. On the slow
path, there is a check_vma_flags() call which returns -EFAULT if the vma
is not readable. So the pte_access_permitted() on the fast path matches
the semantics of the slow path.

If one wants the page structure, FOLL_FORCE ignores the read check (on
the slow path), though I think it still fails if VM_MAYREAD is not set.
Unless you have a real use-case where this is not sufficient, I'd leave
the behaviour as is on arm64 (and maybe update other architectures that
support exec-only to do the same).

-- 
Catalin