diff mbox series

[RFC,7/8] mm: secretmem: use AS_INACCESSIBLE to prohibit GUP

Message ID 20240709132041.3625501-8-roypat@amazon.co.uk (mailing list archive)
State New
Headers show
Series Unmapping guest_memfd from Direct Map | expand

Commit Message

Patrick Roy July 9, 2024, 1:20 p.m. UTC
Inside of vma_is_secretmem and secretmem_mapping, instead of checking
whether a vm_area_struct/address_space has the secretmem ops structure
attached to it, check whether the address_space has the AS_INACCESSIBLE
bit set. Then set the AS_INACCESSIBLE flag for secretmem's
address_space.

This means that get_user_pages and friends are disables for all
adress_spaces that set AS_INACCESIBLE. The AS_INACCESSIBLE flag was
introduced in commit c72ceafbd12c ("mm: Introduce AS_INACCESSIBLE for
encrypted/confidential memory") specifically for guest_memfd to indicate
that no reads and writes should ever be done to guest_memfd
address_spaces. Disallowing gup seems like a reasonable semantic
extension, and means that potential future mmaps of guest_memfd cannot
be GUP'd.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
---
 include/linux/secretmem.h | 13 +++++++++++--
 mm/secretmem.c            |  6 +-----
 2 files changed, 12 insertions(+), 7 deletions(-)

Comments

David Hildenbrand July 9, 2024, 9:09 p.m. UTC | #1
On 09.07.24 15:20, Patrick Roy wrote:
> Inside of vma_is_secretmem and secretmem_mapping, instead of checking
> whether a vm_area_struct/address_space has the secretmem ops structure
> attached to it, check whether the address_space has the AS_INACCESSIBLE
> bit set. Then set the AS_INACCESSIBLE flag for secretmem's
> address_space.
> 
> This means that get_user_pages and friends are disables for all
> adress_spaces that set AS_INACCESIBLE. The AS_INACCESSIBLE flag was
> introduced in commit c72ceafbd12c ("mm: Introduce AS_INACCESSIBLE for
> encrypted/confidential memory") specifically for guest_memfd to indicate
> that no reads and writes should ever be done to guest_memfd
> address_spaces. Disallowing gup seems like a reasonable semantic
> extension, and means that potential future mmaps of guest_memfd cannot
> be GUP'd.
> 
> Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
> ---
>   include/linux/secretmem.h | 13 +++++++++++--
>   mm/secretmem.c            |  6 +-----
>   2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
> index e918f96881f5..886c8f7eb63e 100644
> --- a/include/linux/secretmem.h
> +++ b/include/linux/secretmem.h
> @@ -8,10 +8,19 @@ extern const struct address_space_operations secretmem_aops;
>   
>   static inline bool secretmem_mapping(struct address_space *mapping)
>   {
> -	return mapping->a_ops == &secretmem_aops;
> +	return mapping->flags & AS_INACCESSIBLE;
> +}
> +
> +static inline bool vma_is_secretmem(struct vm_area_struct *vma)
> +{
> +	struct file *file = vma->vm_file;
> +
> +	if (!file)
> +		return false;
> +
> +	return secretmem_mapping(file->f_inode->i_mapping);
>   }

That sounds wrong. You should leave *secretmem alone and instead have 
something like inaccessible_mapping that is used where appropriate.

vma_is_secretmem() should not suddenly succeed on something that is not 
mm/secretmem.c
Mike Rapoport July 10, 2024, 7:32 a.m. UTC | #2
On Tue, Jul 09, 2024 at 11:09:29PM +0200, David Hildenbrand wrote:
> On 09.07.24 15:20, Patrick Roy wrote:
> > Inside of vma_is_secretmem and secretmem_mapping, instead of checking
> > whether a vm_area_struct/address_space has the secretmem ops structure
> > attached to it, check whether the address_space has the AS_INACCESSIBLE
> > bit set. Then set the AS_INACCESSIBLE flag for secretmem's
> > address_space.
> > 
> > This means that get_user_pages and friends are disables for all
> > adress_spaces that set AS_INACCESIBLE. The AS_INACCESSIBLE flag was
> > introduced in commit c72ceafbd12c ("mm: Introduce AS_INACCESSIBLE for
> > encrypted/confidential memory") specifically for guest_memfd to indicate
> > that no reads and writes should ever be done to guest_memfd
> > address_spaces. Disallowing gup seems like a reasonable semantic
> > extension, and means that potential future mmaps of guest_memfd cannot
> > be GUP'd.
> > 
> > Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
> > ---
> >   include/linux/secretmem.h | 13 +++++++++++--
> >   mm/secretmem.c            |  6 +-----
> >   2 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
> > index e918f96881f5..886c8f7eb63e 100644
> > --- a/include/linux/secretmem.h
> > +++ b/include/linux/secretmem.h
> > @@ -8,10 +8,19 @@ extern const struct address_space_operations secretmem_aops;
> >   static inline bool secretmem_mapping(struct address_space *mapping)
> >   {
> > -	return mapping->a_ops == &secretmem_aops;
> > +	return mapping->flags & AS_INACCESSIBLE;
> > +}
> > +
> > +static inline bool vma_is_secretmem(struct vm_area_struct *vma)
> > +{
> > +	struct file *file = vma->vm_file;
> > +
> > +	if (!file)
> > +		return false;
> > +
> > +	return secretmem_mapping(file->f_inode->i_mapping);
> >   }
> 
> That sounds wrong. You should leave *secretmem alone and instead have
> something like inaccessible_mapping that is used where appropriate.
> 
> vma_is_secretmem() should not suddenly succeed on something that is not
> mm/secretmem.c

I'm with David here.
 
> -- 
> Cheers,
> 
> David / dhildenb
>
Patrick Roy July 10, 2024, 9:50 a.m. UTC | #3
On 7/10/24 08:32, Mike Rapoport wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Tue, Jul 09, 2024 at 11:09:29PM +0200, David Hildenbrand wrote:
>> On 09.07.24 15:20, Patrick Roy wrote:
>>> Inside of vma_is_secretmem and secretmem_mapping, instead of checking
>>> whether a vm_area_struct/address_space has the secretmem ops structure
>>> attached to it, check whether the address_space has the AS_INACCESSIBLE
>>> bit set. Then set the AS_INACCESSIBLE flag for secretmem's
>>> address_space.
>>>
>>> This means that get_user_pages and friends are disables for all
>>> adress_spaces that set AS_INACCESIBLE. The AS_INACCESSIBLE flag was
>>> introduced in commit c72ceafbd12c ("mm: Introduce AS_INACCESSIBLE for
>>> encrypted/confidential memory") specifically for guest_memfd to indicate
>>> that no reads and writes should ever be done to guest_memfd
>>> address_spaces. Disallowing gup seems like a reasonable semantic
>>> extension, and means that potential future mmaps of guest_memfd cannot
>>> be GUP'd.
>>>
>>> Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
>>> ---
>>>   include/linux/secretmem.h | 13 +++++++++++--
>>>   mm/secretmem.c            |  6 +-----
>>>   2 files changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
>>> index e918f96881f5..886c8f7eb63e 100644
>>> --- a/include/linux/secretmem.h
>>> +++ b/include/linux/secretmem.h
>>> @@ -8,10 +8,19 @@ extern const struct address_space_operations secretmem_aops;
>>>   static inline bool secretmem_mapping(struct address_space *mapping)
>>>   {
>>> -   return mapping->a_ops == &secretmem_aops;
>>> +   return mapping->flags & AS_INACCESSIBLE;
>>> +}
>>> +
>>> +static inline bool vma_is_secretmem(struct vm_area_struct *vma)
>>> +{
>>> +   struct file *file = vma->vm_file;
>>> +
>>> +   if (!file)
>>> +           return false;
>>> +
>>> +   return secretmem_mapping(file->f_inode->i_mapping);
>>>   }
>>
>> That sounds wrong. You should leave *secretmem alone and instead have
>> something like inaccessible_mapping that is used where appropriate.
>>
>> vma_is_secretmem() should not suddenly succeed on something that is not
>> mm/secretmem.c
> 
> I'm with David here.
> 

Right, that makes sense. My thinking here was that if memfd_secret and
potential mappings of guest_memfd have the same behavior wrt GUP, then
it makes sense to just have them rely on the same checks. But I guess I
didn't follow that thought to its logical conclusion of renaming the
"secretmem" checks into "inaccessible" checks and moving them out of
secretmem.h.

Or do you mean to just leave secretmem untouched and add separate
"inaccessible" checks? But then we'd have two different ways of
disabling GUP for specific VMAs that both rely on checks in exactly the
same places :/

>> --
>> Cheers,
>>
>> David / dhildenb
>>
> 
> --
> Sincerely yours,
> Mike.
David Hildenbrand July 10, 2024, 9:14 p.m. UTC | #4
On 10.07.24 11:50, Patrick Roy wrote:
> 
> 
> On 7/10/24 08:32, Mike Rapoport wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On Tue, Jul 09, 2024 at 11:09:29PM +0200, David Hildenbrand wrote:
>>> On 09.07.24 15:20, Patrick Roy wrote:
>>>> Inside of vma_is_secretmem and secretmem_mapping, instead of checking
>>>> whether a vm_area_struct/address_space has the secretmem ops structure
>>>> attached to it, check whether the address_space has the AS_INACCESSIBLE
>>>> bit set. Then set the AS_INACCESSIBLE flag for secretmem's
>>>> address_space.
>>>>
>>>> This means that get_user_pages and friends are disables for all
>>>> adress_spaces that set AS_INACCESIBLE. The AS_INACCESSIBLE flag was
>>>> introduced in commit c72ceafbd12c ("mm: Introduce AS_INACCESSIBLE for
>>>> encrypted/confidential memory") specifically for guest_memfd to indicate
>>>> that no reads and writes should ever be done to guest_memfd
>>>> address_spaces. Disallowing gup seems like a reasonable semantic
>>>> extension, and means that potential future mmaps of guest_memfd cannot
>>>> be GUP'd.
>>>>
>>>> Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
>>>> ---
>>>>    include/linux/secretmem.h | 13 +++++++++++--
>>>>    mm/secretmem.c            |  6 +-----
>>>>    2 files changed, 12 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
>>>> index e918f96881f5..886c8f7eb63e 100644
>>>> --- a/include/linux/secretmem.h
>>>> +++ b/include/linux/secretmem.h
>>>> @@ -8,10 +8,19 @@ extern const struct address_space_operations secretmem_aops;
>>>>    static inline bool secretmem_mapping(struct address_space *mapping)
>>>>    {
>>>> -   return mapping->a_ops == &secretmem_aops;
>>>> +   return mapping->flags & AS_INACCESSIBLE;
>>>> +}
>>>> +
>>>> +static inline bool vma_is_secretmem(struct vm_area_struct *vma)
>>>> +{
>>>> +   struct file *file = vma->vm_file;
>>>> +
>>>> +   if (!file)
>>>> +           return false;
>>>> +
>>>> +   return secretmem_mapping(file->f_inode->i_mapping);
>>>>    }
>>>
>>> That sounds wrong. You should leave *secretmem alone and instead have
>>> something like inaccessible_mapping that is used where appropriate.
>>>
>>> vma_is_secretmem() should not suddenly succeed on something that is not
>>> mm/secretmem.c
>>
>> I'm with David here.
>>
> 
> Right, that makes sense. My thinking here was that if memfd_secret and
> potential mappings of guest_memfd have the same behavior wrt GUP, then
> it makes sense to just have them rely on the same checks. But I guess I
> didn't follow that thought to its logical conclusion of renaming the
> "secretmem" checks into "inaccessible" checks and moving them out of
> secretmem.h.
> 
> Or do you mean to just leave secretmem untouched and add separate
> "inaccessible" checks? But then we'd have two different ways of
> disabling GUP for specific VMAs that both rely on checks in exactly the
> same places :/

You can just replace the vma_is_secretmem in relevant places by checks 
if inaccessible address spaces. No need for the additional 
vma_is_secretmem check then.

BUT, as raised in my other reply, I wonder if adding support for 
secretmem in KVM (I assume) would be simpler+cleaner.

> 
>>> --
>>> Cheers,
>>>
>>> David / dhildenb
>>>
>>
>> --
>> Sincerely yours,
>> Mike.
>
diff mbox series

Patch

diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
index e918f96881f5..886c8f7eb63e 100644
--- a/include/linux/secretmem.h
+++ b/include/linux/secretmem.h
@@ -8,10 +8,19 @@  extern const struct address_space_operations secretmem_aops;
 
 static inline bool secretmem_mapping(struct address_space *mapping)
 {
-	return mapping->a_ops == &secretmem_aops;
+	return mapping->flags & AS_INACCESSIBLE;
+}
+
+static inline bool vma_is_secretmem(struct vm_area_struct *vma)
+{
+	struct file *file = vma->vm_file;
+
+	if (!file)
+		return false;
+
+	return secretmem_mapping(file->f_inode->i_mapping);
 }
 
-bool vma_is_secretmem(struct vm_area_struct *vma);
 bool secretmem_active(void);
 
 #else
diff --git a/mm/secretmem.c b/mm/secretmem.c
index 3afb5ad701e1..fd03a84a1cb5 100644
--- a/mm/secretmem.c
+++ b/mm/secretmem.c
@@ -136,11 +136,6 @@  static int secretmem_mmap(struct file *file, struct vm_area_struct *vma)
 	return 0;
 }
 
-bool vma_is_secretmem(struct vm_area_struct *vma)
-{
-	return vma->vm_ops == &secretmem_vm_ops;
-}
-
 static const struct file_operations secretmem_fops = {
 	.release	= secretmem_release,
 	.mmap		= secretmem_mmap,
@@ -218,6 +213,7 @@  static struct file *secretmem_file_create(unsigned long flags)
 
 	inode->i_op = &secretmem_iops;
 	inode->i_mapping->a_ops = &secretmem_aops;
+	inode->i_mapping->flags |= AS_INACCESSIBLE;
 
 	/* pretend we are a normal file with zero size */
 	inode->i_mode |= S_IFREG;