memfd: deny writeable mappings when implying SEAL_WRITE
Subsystems (3)
PATCH 1/1 memfd: deny writeable mappings when implying SEAL_WRITE
From: "Pratyush Yadav (Google)" <[email protected]>
When SEAL_EXEC is added, SEAL_WRITE is implied to make W^X. But the implied seal is set after the check that makes sure the memfd can not have any writable mappings. This means one can use SEAL_EXEC to apply SEAL_WRITE while having writeable mappings.
This breaks the contract that SEAL_WRITE provides and can be used by an attacker to pass a memfd that appears to be write sealed but can still be modified arbitrarily.
Fix this by adding the implied seals before the call for mapping_deny_writable() is done.
--- mm/memfd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
mm/memfd.c +6 -6
@@ -283,6 +283,12 @@ int memfd_add_seals(struct file *file, unsigned int seals) goto unlock; } +/* +* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. +*/ +if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) + seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; +if ((seals &F_SEAL_WRITE ) && !(*file_seals &F_SEAL_WRITE )) { error =mapping_deny_writable (file->f_mapping);if (error)
@@ -295,12 +301,6 @@ int memfd_add_seals(struct file *file, unsigned int seals) } } -/* -* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. -*/ -if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) - seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; - *file_seals |= seals; error =0 ;
Signature · 1 line
2.54.0.545.g6539524ca2-goog
Quoted 11 lines · Pratyush Yadav
From: "Pratyush Yadav (Google)" <[email protected]>
When
SEAL_EXECis added,SEAL_WRITEis implied to make W^X. But the implied seal is set after the check that makes sure the memfd can not have any writable mappings. This means one can useSEAL_EXECto applySEAL_WRITEwhile having writeable mappings.This breaks the contract that
SEAL_WRITEprovides and can be used by an attacker to pass a memfd that appears to be write sealed but can still be modified arbitrarily.
Quoted 6 lines
Fix this by adding the implied seals before the call for
mapping_deny_writable()is done.Fixes c4f75bc8bd6b ("mm/memfd: add write seals when apply SEAL_EXEC to executable memfd")Signed-off-by Pratyush Yadav (Google) <[email protected]>
Quoted 37 lines
--- mm/memfd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)mm/memfd.c +6
-6@@ -283,6 +283,12 @@ int memfd_add_seals(struct file *file, unsigned int seals) goto unlock; } +/* +* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. +*/ +if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) + seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; +if ((seals &F_SEAL_WRITE ) && !(*file_seals &F_SEAL_WRITE )) { error =mapping_deny_writable (file->f_mapping);if (error)@@ -295,12 +301,6 @@ int memfd_add_seals(struct file *file, unsigned int seals) } } -/* -* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. -*/ -if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) - seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; - *file_seals |= seals; error =0 ;Signature · 1 line
2.54.0.545.g6539524ca2-goog
Pasha Tatashin
AppliedQuoted 10 lines · Pratyush Yadav
When
SEAL_EXECis added,SEAL_WRITEis implied to make W^X. But the implied seal is set after the check that makes sure the memfd can not have any writable mappings. This means one can useSEAL_EXECto applySEAL_WRITEwhile having writeable mappings.This breaks the contract that
SEAL_WRITEprovides and can be used by an attacker to pass a memfd that appears to be write sealed but can still be modified arbitrarily.[...]
Applied, thanks!
[1/1] memfd: deny writeable mappings when implying SEAL_WRITE
commit: 73f496662a9848021e75742a69a3239ea850c3ee
Best regards,
Signature · 1 line
Pasha Tatashin <[email protected]>
Quoted 11 lines · Pasha Tatashin
Quoted 9 lines · Pratyush Yadav
When
SEAL_EXECis added,SEAL_WRITEis implied to make W^X. But the implied seal is set after the check that makes sure the memfd can not have any writable mappings. This means one can useSEAL_EXECto applySEAL_WRITEwhile having writeable mappings.This breaks the contract that
SEAL_WRITEprovides and can be used by an attacker to pass a memfd that appears to be write sealed but can still be modified arbitrarily.
Quoted 6 lines
Quoted 1 line
[...]
Applied, thanks!
[1/1] memfd: deny writeable mappings when implying
SEAL_WRITEcommit: 73f496662a9848021e75742a69a3239ea850c3ee
^^^
Please ignore, this should be Applied to MM tree.
Pasha
Quoted 5 lines
Best regards,
Signature · 1 line
Pasha Tatashin <[email protected]>
Hi Pratyush,
Thank you for fixing this.
Quoted 12 lines · Pratyush Yadav
From: "Pratyush Yadav (Google)" <[email protected]>
When
SEAL_EXECis added,SEAL_WRITEis implied to make W^X. But the implied seal is set after the check that makes sure the memfd can not have any writable mappings. This means one can useSEAL_EXECto applySEAL_WRITEwhile having writeable mappings.This breaks the contract that
SEAL_WRITEprovides and can be used by an attacker to pass a memfd that appears to be write sealed but can still be modified arbitrarily.
Quoted 6 lines
Fix this by adding the implied seals before the call for
mapping_deny_writable()is done.Fixes c4f75bc8bd6b ("mm/memfd: add write seals when apply SEAL_EXEC to executable memfd")Signed-off-by Pratyush Yadav (Google) <[email protected]>
-Jeff
Quoted 37 lines
--- mm/memfd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)mm/memfd.c +6
-7@@ -283,6 +283,12 @@ int memfd_add_seals(struct file *file, unsigned int seals) goto unlock; } +/* +* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. +*/ +if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) + seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; +if ((seals &F_SEAL_WRITE ) && !(*file_seals &F_SEAL_WRITE )) { error =mapping_deny_writable (file->f_mapping);if (error)@@ -295,12 +301,6 @@ int memfd_add_seals(struct file *file, unsigned int seals) } } -/* -* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. -*/ -if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) - seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; - *file_seals |= seals; error =0 ; --2.54.0.545.g6539524ca2-goog
Quoted 3 lines · Pratyush Yadav
From: "Pratyush Yadav (Google)" <[email protected]>
When
SEAL_EXECis added,SEAL_WRITEis implied to make W^X.
I don't quite understand that.
I guess what you mean is "SEAL_EXEC implies SEAL_WRITE if the file is executable, to prevent W^X after sealing".
Because if the file is not executable, there is no sealing of writes happening?
It's rather odd to combine both things, though. Likely the callers should just have requested SEAL_WRITE.
But I guess we are stuck with this mess.
Quoted 39 lines
But the
implied seal is set after the check that makes sure the memfd can not have any writable mappings. This means one can useSEAL_EXECto applySEAL_WRITEwhile having writeable mappings.This breaks the contract that
SEAL_WRITEprovides and can be used by an attacker to pass a memfd that appears to be write sealed but can still be modified arbitrarily.Fix this by adding the implied seals before the call for
mapping_deny_writable()is done.Fixes c4f75bc8bd6b ("mm/memfd: add write seals when apply SEAL_EXEC to executable memfd")Signed-off-by Pratyush Yadav (Google) <[email protected]>--- mm/memfd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)mm/memfd.c +6
-0@@ -283,6 +283,12 @@ int memfd_add_seals(struct file *file, unsigned int seals) goto unlock; } +/* +* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. +*/ +if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) + seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; +if ((seals &F_SEAL_WRITE ) && !(*file_seals &F_SEAL_WRITE )) { error =mapping_deny_writable (file->f_mapping);if (error)@@ -295,12 +301,6 @@ int memfd_add_seals(struct file *file, unsigned int seals) } }
mm/memfd.c int memfd_add_seals(struct file *file, unsigned int seals) · line 306
Quoted 10 lines
mm/memfd.c +0
-6-/* -* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. -*/ -if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) - seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; - *file_seals |= seals; error =0 ;
Given the weird semantics, this makes sense to me.
Do we have to update documentation to reflect this? But staring at the man page
[1] we don't even seem to document F_SEAL_EXEC?
[1] https://www.man7.org/linux/man-pages/man2/F_ADD_SEALS.2const.html
Signature · 3 lines
Cheers, David
Quoted 9 lines · David Hildenbrand (Arm)
Quoted 3 lines · Pratyush Yadav
From: "Pratyush Yadav (Google)" <[email protected]>
When
SEAL_EXECis added,SEAL_WRITEis implied to make W^X.I don't quite understand that.
I guess what you mean is "
SEAL_EXECimpliesSEAL_WRITEif the file is executable, to prevent W^X after sealing".
Yes, exactly. If the file is executable and SEAL_EXEC is set, SEAL_WRITE is also set to make sure the executable code is not writeable.
Quoted 7 lines
Because if the file is not executable, there is no sealing of writes happening?
It's rather odd to combine both things, though. Likely the callers should just have requested
SEAL_WRITE.But I guess we are stuck with this mess.
Yep :-/
Quoted 54 lines
Quoted 41 lines
But the
implied seal is set after the check that makes sure the memfd can not have any writable mappings. This means one can useSEAL_EXECto applySEAL_WRITEwhile having writeable mappings.This breaks the contract that
SEAL_WRITEprovides and can be used by an attacker to pass a memfd that appears to be write sealed but can still be modified arbitrarily.Fix this by adding the implied seals before the call for
mapping_deny_writable()is done.Fixes c4f75bc8bd6b ("mm/memfd: add write seals when apply SEAL_EXEC to executable memfd")Signed-off-by Pratyush Yadav (Google) <[email protected]>--- mm/memfd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)mm/memfd.c +6
-0@@ -283,6 +283,12 @@ int memfd_add_seals(struct file *file, unsigned int seals) goto unlock; } +/* +* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. +*/ +if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) + seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; +if ((seals &F_SEAL_WRITE ) && !(*file_seals &F_SEAL_WRITE )) { error =mapping_deny_writable (file->f_mapping);if (error)@@ -295,12 +301,6 @@ int memfd_add_seals(struct file *file, unsigned int seals) } }mm/memfd.c int memfd_add_seals(struct file *file, unsigned int seals) · line 306
Quoted 10 lines
mm/memfd.c +0
-6-/* -* SEAL_EXEC implies SEAL_WRITE, making W^X from the start. -*/ -if (seals &F_SEAL_EXEC && inode->i_mode &0111 ) - seals |=F_SEAL_SHRINK |F_SEAL_GROW |F_SEAL_WRITE |F_SEAL_FUTURE_WRITE ; - *file_seals |= seals; error =0 ;Given the weird semantics, this makes sense to me.
Quoted 2 lines
Do we have to update documentation to reflect this? But staring at the man page
[1] we don't even seem to documentF_SEAL_EXEC?
I discovered the same when trying to read more about F_SEAL_EXEC. I've never written man pages but I suppose I can give this a shot.
Quoted 3 lines
[1] https://www.man7.org/linux/man-pages/man2/F_ADD_SEALS.2const.html
Signature · 2 lines
Regards, Pratyush Yadav