get_user_pages() and EXEC_ONLY mapping.

get_user_pages() and EXEC_ONLY mapping.

[Aneesh Kumar K.V]

Hello,

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?

-aneesh

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: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.

[Aneesh Kumar K V]

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  

"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. "

May be we can clarify PROT_EXEC details in the documentation? 

-aneesh

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.

[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

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