include/linux/mm.h | 2 ++ include/linux/security.h | 8 +++---- ipc/shm.c | 2 +- mm/mmap.c | 42 ++++++++++++++++++++++------------ mm/nommu.c | 49 +++++++++++++++++++++++++++++++--------- mm/util.c | 2 +- security/security.c | 41 ++++----------------------------- 7 files changed, 78 insertions(+), 68 deletions(-)
This patch moves the check of READ_IMPLIES_EXEC out of do_mmap(), and
calls the LSM hooks at the same time. The below is reason.
Some logic may want to call do_mmap(), but they are not aware of
the harm this poses to LSM.
For example, CVE-2016-10044[1] has reported many years ago, but the
remap_file_pages() can still bypass W^X policy enforced by SELinux[2].
Add a check is easy, but there may have more calls to do_mmap() in the
furture. And move the security check in do_mmap() will let it in the
mmap_write_lock, which slow down the performance and even have deadlocks
if someone depends on it.
Moving the check of READ_IMPLIES_EXEC out of do_mmap() can keep the LSM
hook is called out of the mmap_write_lock, and avoid repeated logic for
whether READ_IMPLIES_EXEC should add PROT_EXEC for the mapping or not(In
current, the !MMU case won't implies exec if file's mmap_capabilities is
not exist, but the security check logic is different). And all known cases
that call do_mmap() become secure and anyone want to apply the
READ_IMPLIES_EXEC logic will call the LSM hook correctly.
Current call sites to do_mmap():
arch/x86/kernel/shstk.c: must not want to PROT_EXEC in any case.
fs/aio.c: have ensured never have PROT_EXEC in the fix of CVE-2016-10044.
mm/util.c and ipc/shm.c: the common case
mm/mmap.c: in the history, remap_file_pages won't care about the
READ_IMPLIES_EXEC. this side effect is introduced in the emulation
version, after the deprecated mark exist.
Link: https://project-zero.issues.chromium.org/issues/42452389 [1]
Link: https://lore.kernel.org/all/20240919080905.4506-2-paul@paul-moore.com/ [2]
Signed-off-by: Shu Han <ebpqwerty472123@gmail.com>
---
An alternative method is moving security_file_mmap() back into do_mmap(),
i.e. revert the commit 8b3ec6814c83d76b85bd13badc48552836c24839
("take security_mmap_file() outside of ->mmap_sem"), which is simpler,
but it will affect performance.
---
include/linux/mm.h | 2 ++
include/linux/security.h | 8 +++----
ipc/shm.c | 2 +-
mm/mmap.c | 42 ++++++++++++++++++++++------------
mm/nommu.c | 49 +++++++++++++++++++++++++++++++---------
mm/util.c | 2 +-
security/security.c | 41 ++++-----------------------------
7 files changed, 78 insertions(+), 68 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c4b238a20b76..83f334590b06 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3392,6 +3392,8 @@ get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
extern unsigned long mmap_region(struct file *file, unsigned long addr,
unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
struct list_head *uf);
+extern int mmap_check_prot(struct file *file, unsigned long *prot,
+ unsigned long flags);
extern unsigned long do_mmap(struct file *file, unsigned long addr,
unsigned long len, unsigned long prot, unsigned long flags,
vm_flags_t vm_flags, unsigned long pgoff, unsigned long *populate,
diff --git a/include/linux/security.h b/include/linux/security.h
index c37c32ebbdcd..e061bc9a0331 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -423,8 +423,8 @@ void security_file_free(struct file *file);
int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
int security_file_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg);
-int security_mmap_file(struct file *file, unsigned long prot,
- unsigned long flags);
+int security_mmap_file(struct file *file, unsigned long reqprot,
+ unsigned long prot, unsigned long flags);
int security_mmap_addr(unsigned long addr);
int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
unsigned long prot);
@@ -1077,8 +1077,8 @@ static inline int security_file_ioctl_compat(struct file *file,
return 0;
}
-static inline int security_mmap_file(struct file *file, unsigned long prot,
- unsigned long flags)
+static inline int security_mmap_file(struct file *file, unsigned long reqprot,
+ unsigned long prot, unsigned long flags)
{
return 0;
}
diff --git a/ipc/shm.c b/ipc/shm.c
index 3e3071252dac..f1095ee3796d 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1636,7 +1636,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
sfd->vm_ops = NULL;
file->private_data = sfd;
- err = security_mmap_file(file, prot, flags);
+ err = mmap_check_prot(file, &prot, flags);
if (err)
goto out_fput;
diff --git a/mm/mmap.c b/mm/mmap.c
index 18fddcce03b8..b8992ee202fe 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1249,8 +1249,36 @@ static inline bool file_mmap_ok(struct file *file, struct inode *inode,
return true;
}
+int mmap_check_prot(struct file *file, unsigned long *prot,
+ unsigned long flags)
+{
+ unsigned long req_prot = *prot;
+ unsigned long new_prot = req_prot;
+ int err;
+
+ /*
+ * Does the application expect PROT_READ to imply PROT_EXEC?
+ *
+ * (the exception is when the underlying filesystem is noexec
+ * mounted, in which case we don't add PROT_EXEC.)
+ */
+ if (((req_prot & (PROT_READ | PROT_EXEC)) == PROT_READ) &&
+ (current->personality & READ_IMPLIES_EXEC) &&
+ !(file && path_noexec(&file->f_path)))
+ new_prot |= PROT_EXEC;
+
+ err = security_mmap_file(file, req_prot, new_prot, flags);
+ if (err)
+ return err;
+
+ *prot = new_prot;
+ return 0;
+}
+
/*
* The caller must write-lock current->mm->mmap_lock.
+ * The caller must ensure security check is not needed or
+ * call to mmap_check_prot before.
*/
unsigned long do_mmap(struct file *file, unsigned long addr,
unsigned long len, unsigned long prot,
@@ -1266,16 +1294,6 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
if (!len)
return -EINVAL;
- /*
- * Does the application expect PROT_READ to imply PROT_EXEC?
- *
- * (the exception is when the underlying filesystem is noexec
- * mounted, in which case we don't add PROT_EXEC.)
- */
- if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
- if (!(file && path_noexec(&file->f_path)))
- prot |= PROT_EXEC;
-
/* force arch specific MAP_FIXED handling in get_unmapped_area */
if (flags & MAP_FIXED_NOREPLACE)
flags |= MAP_FIXED;
@@ -3198,12 +3216,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
flags |= MAP_LOCKED;
file = get_file(vma->vm_file);
- ret = security_mmap_file(vma->vm_file, prot, flags);
- if (ret)
- goto out_fput;
ret = do_mmap(vma->vm_file, start, size,
prot, flags, 0, pgoff, &populate, NULL);
-out_fput:
fput(file);
out:
mmap_write_unlock(mm);
diff --git a/mm/nommu.c b/mm/nommu.c
index 7296e775e04e..96761add1295 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -792,12 +792,6 @@ static int validate_mmap_request(struct file *file,
if (path_noexec(&file->f_path)) {
if (prot & PROT_EXEC)
return -EPERM;
- } else if ((prot & PROT_READ) && !(prot & PROT_EXEC)) {
- /* handle implication of PROT_EXEC by PROT_READ */
- if (current->personality & READ_IMPLIES_EXEC) {
- if (capabilities & NOMMU_MAP_EXEC)
- prot |= PROT_EXEC;
- }
} else if ((prot & PROT_READ) &&
(prot & PROT_EXEC) &&
!(capabilities & NOMMU_MAP_EXEC)
@@ -810,11 +804,6 @@ static int validate_mmap_request(struct file *file,
* privately mapped
*/
capabilities = NOMMU_MAP_COPY;
-
- /* handle PROT_EXEC implication by PROT_READ */
- if ((prot & PROT_READ) &&
- (current->personality & READ_IMPLIES_EXEC))
- prot |= PROT_EXEC;
}
/* allow the security API to have its say */
@@ -992,6 +981,44 @@ static int do_mmap_private(struct vm_area_struct *vma,
return -ENOMEM;
}
+int mmap_check_prot(struct file *file, unsigned long *prot,
+ unsigned long flags)
+{
+ unsigned long req_prot = *prot;
+ unsigned long new_prot = req_prot;
+ int err;
+
+ /*
+ * Does the application expect PROT_READ to imply PROT_EXEC?
+ *
+ * (the exception is when the underlying filesystem is noexec
+ * mounted or the file does not have NOMMU_MAP_EXEC
+ * (== VM_MAYEXEC), in which case we don't add PROT_EXEC.)
+ */
+ if ((req_prot & (PROT_READ | PROT_EXEC)) != PROT_READ)
+ goto check;
+ if (!(current->personality & READ_IMPLIES_EXEC))
+ goto check;
+ if (!file) {
+ new_prot |= PROT_EXEC;
+ goto check;
+ }
+ if (file->f_op->mmap_capabilities) {
+ unsigned int caps = file->f_op->mmap_capabilities(file);
+
+ if (!(caps & NOMMU_MAP_EXEC))
+ goto check;
+ new_prot |= PROT_EXEC;
+ }
+check:
+ err = security_mmap_file(file, req_prot, new_prot, flags);
+ if (err)
+ return err;
+
+ *prot = new_prot;
+ return 0;
+}
+
/*
* handle mapping creation for uClinux
*/
diff --git a/mm/util.c b/mm/util.c
index bd283e2132e0..2eb4d6037610 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -581,7 +581,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
unsigned long populate;
LIST_HEAD(uf);
- ret = security_mmap_file(file, prot, flag);
+ ret = mmap_check_prot(file, &prot, flag);
if (!ret) {
if (mmap_write_lock_killable(mm))
return -EINTR;
diff --git a/security/security.c b/security/security.c
index 4564a0a1e4ef..25556629f588 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2927,42 +2927,10 @@ int security_file_ioctl_compat(struct file *file, unsigned int cmd,
}
EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
-static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
-{
- /*
- * Does we have PROT_READ and does the application expect
- * it to imply PROT_EXEC? If not, nothing to talk about...
- */
- if ((prot & (PROT_READ | PROT_EXEC)) != PROT_READ)
- return prot;
- if (!(current->personality & READ_IMPLIES_EXEC))
- return prot;
- /*
- * if that's an anonymous mapping, let it.
- */
- if (!file)
- return prot | PROT_EXEC;
- /*
- * ditto if it's not on noexec mount, except that on !MMU we need
- * NOMMU_MAP_EXEC (== VM_MAYEXEC) in this case
- */
- if (!path_noexec(&file->f_path)) {
-#ifndef CONFIG_MMU
- if (file->f_op->mmap_capabilities) {
- unsigned caps = file->f_op->mmap_capabilities(file);
- if (!(caps & NOMMU_MAP_EXEC))
- return prot;
- }
-#endif
- return prot | PROT_EXEC;
- }
- /* anything on noexec mount won't get PROT_EXEC */
- return prot;
-}
-
/**
* security_mmap_file() - Check if mmap'ing a file is allowed
* @file: file
+ * @reqprot: protection requested by user
* @prot: protection applied by the kernel
* @flags: flags
*
@@ -2971,11 +2939,10 @@ static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
*
* Return: Returns 0 if permission is granted.
*/
-int security_mmap_file(struct file *file, unsigned long prot,
- unsigned long flags)
+int security_mmap_file(struct file *file, unsigned long reqprot,
+ unsigned long prot, unsigned long flags)
{
- return call_int_hook(mmap_file, file, prot, mmap_prot(file, prot),
- flags);
+ return call_int_hook(mmap_file, file, reqprot, prot, flags);
}
/**
base-commit: f89722faa31466ff41aed21bdeb9cf34c2312858
--
2.34.1NACK.
You have sent this non-RFC intentionally conflicting with [0] to provide
'alternatives' that is not what a [PATCH] submission is.
It's a bit weird to send 'alternatives' - you should by now have a good
sense of which ought to work, if not perhaps more research is required on
your part?
In any case, speculative changes like this should ABSOLUTELY be sent RFC,
and sending things that are merge conflicts as ordinary patches is actually
bordering on being a little rude!
I'm sure it's unintentional :) but for the sake of us being able to work
with these properly you should just send one as RFC and ask whether it'd be
appropriate to send an alternative, and just allude to it in the one you do
send.
[0]:https://lore.kernel.org/all/20240925081628.408-1-ebpqwerty472123@gmail.com/
Also +Al whose original patch you are in effect reverting.
On Wed, Sep 25, 2024 at 02:30:34PM GMT, Shu Han wrote:
> This patch moves the check of READ_IMPLIES_EXEC out of do_mmap(), and
> calls the LSM hooks at the same time. The below is reason.
>
> Some logic may want to call do_mmap(), but they are not aware of
> the harm this poses to LSM.
>
> For example, CVE-2016-10044[1] has reported many years ago, but the
> remap_file_pages() can still bypass W^X policy enforced by SELinux[2].
>
> Add a check is easy, but there may have more calls to do_mmap() in the
> furture. And move the security check in do_mmap() will let it in the
> mmap_write_lock, which slow down the performance and even have deadlocks
> if someone depends on it.
>
> Moving the check of READ_IMPLIES_EXEC out of do_mmap() can keep the LSM
> hook is called out of the mmap_write_lock, and avoid repeated logic for
> whether READ_IMPLIES_EXEC should add PROT_EXEC for the mapping or not(In
> current, the !MMU case won't implies exec if file's mmap_capabilities is
> not exist, but the security check logic is different). And all known cases
> that call do_mmap() become secure and anyone want to apply the
> READ_IMPLIES_EXEC logic will call the LSM hook correctly.
>
> Current call sites to do_mmap():
> arch/x86/kernel/shstk.c: must not want to PROT_EXEC in any case.
> fs/aio.c: have ensured never have PROT_EXEC in the fix of CVE-2016-10044.
> mm/util.c and ipc/shm.c: the common case
> mm/mmap.c: in the history, remap_file_pages won't care about the
> READ_IMPLIES_EXEC. this side effect is introduced in the emulation
> version, after the deprecated mark exist.
>
> Link: https://project-zero.issues.chromium.org/issues/42452389 [1]
> Link: https://lore.kernel.org/all/20240919080905.4506-2-paul@paul-moore.com/ [2]
> Signed-off-by: Shu Han <ebpqwerty472123@gmail.com>
> ---
> An alternative method is moving security_file_mmap() back into do_mmap(),
> i.e. revert the commit 8b3ec6814c83d76b85bd13badc48552836c24839
> ("take security_mmap_file() outside of ->mmap_sem"), which is simpler,
> but it will affect performance.
> ---
> include/linux/mm.h | 2 ++
> include/linux/security.h | 8 +++----
> ipc/shm.c | 2 +-
> mm/mmap.c | 42 ++++++++++++++++++++++------------
> mm/nommu.c | 49 +++++++++++++++++++++++++++++++---------
> mm/util.c | 2 +-
> security/security.c | 41 ++++-----------------------------
> 7 files changed, 78 insertions(+), 68 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c4b238a20b76..83f334590b06 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3392,6 +3392,8 @@ get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
> extern unsigned long mmap_region(struct file *file, unsigned long addr,
> unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
> struct list_head *uf);
> +extern int mmap_check_prot(struct file *file, unsigned long *prot,
> + unsigned long flags);
> extern unsigned long do_mmap(struct file *file, unsigned long addr,
> unsigned long len, unsigned long prot, unsigned long flags,
> vm_flags_t vm_flags, unsigned long pgoff, unsigned long *populate,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c37c32ebbdcd..e061bc9a0331 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -423,8 +423,8 @@ void security_file_free(struct file *file);
> int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> int security_file_ioctl_compat(struct file *file, unsigned int cmd,
> unsigned long arg);
> -int security_mmap_file(struct file *file, unsigned long prot,
> - unsigned long flags);
> +int security_mmap_file(struct file *file, unsigned long reqprot,
> + unsigned long prot, unsigned long flags);
> int security_mmap_addr(unsigned long addr);
> int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> unsigned long prot);
> @@ -1077,8 +1077,8 @@ static inline int security_file_ioctl_compat(struct file *file,
> return 0;
> }
>
> -static inline int security_mmap_file(struct file *file, unsigned long prot,
> - unsigned long flags)
> +static inline int security_mmap_file(struct file *file, unsigned long reqprot,
> + unsigned long prot, unsigned long flags)
> {
> return 0;
> }
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 3e3071252dac..f1095ee3796d 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1636,7 +1636,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
> sfd->vm_ops = NULL;
> file->private_data = sfd;
>
> - err = security_mmap_file(file, prot, flags);
> + err = mmap_check_prot(file, &prot, flags);
> if (err)
> goto out_fput;
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 18fddcce03b8..b8992ee202fe 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1249,8 +1249,36 @@ static inline bool file_mmap_ok(struct file *file, struct inode *inode,
> return true;
> }
>
> +int mmap_check_prot(struct file *file, unsigned long *prot,
> + unsigned long flags)
> +{
> + unsigned long req_prot = *prot;
> + unsigned long new_prot = req_prot;
> + int err;
> +
> + /*
> + * Does the application expect PROT_READ to imply PROT_EXEC?
> + *
> + * (the exception is when the underlying filesystem is noexec
> + * mounted, in which case we don't add PROT_EXEC.)
> + */
> + if (((req_prot & (PROT_READ | PROT_EXEC)) == PROT_READ) &&
> + (current->personality & READ_IMPLIES_EXEC) &&
> + !(file && path_noexec(&file->f_path)))
> + new_prot |= PROT_EXEC;
> +
> + err = security_mmap_file(file, req_prot, new_prot, flags);
> + if (err)
> + return err;
> +
> + *prot = new_prot;
> + return 0;
> +}
> +
> /*
> * The caller must write-lock current->mm->mmap_lock.
> + * The caller must ensure security check is not needed or
> + * call to mmap_check_prot before.
> */
> unsigned long do_mmap(struct file *file, unsigned long addr,
> unsigned long len, unsigned long prot,
> @@ -1266,16 +1294,6 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> if (!len)
> return -EINVAL;
>
> - /*
> - * Does the application expect PROT_READ to imply PROT_EXEC?
> - *
> - * (the exception is when the underlying filesystem is noexec
> - * mounted, in which case we don't add PROT_EXEC.)
> - */
> - if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
> - if (!(file && path_noexec(&file->f_path)))
> - prot |= PROT_EXEC;
> -
> /* force arch specific MAP_FIXED handling in get_unmapped_area */
> if (flags & MAP_FIXED_NOREPLACE)
> flags |= MAP_FIXED;
> @@ -3198,12 +3216,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
> flags |= MAP_LOCKED;
>
> file = get_file(vma->vm_file);
> - ret = security_mmap_file(vma->vm_file, prot, flags);
> - if (ret)
> - goto out_fput;
> ret = do_mmap(vma->vm_file, start, size,
> prot, flags, 0, pgoff, &populate, NULL);
> -out_fput:
> fput(file);
> out:
> mmap_write_unlock(mm);
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 7296e775e04e..96761add1295 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -792,12 +792,6 @@ static int validate_mmap_request(struct file *file,
> if (path_noexec(&file->f_path)) {
> if (prot & PROT_EXEC)
> return -EPERM;
> - } else if ((prot & PROT_READ) && !(prot & PROT_EXEC)) {
> - /* handle implication of PROT_EXEC by PROT_READ */
> - if (current->personality & READ_IMPLIES_EXEC) {
> - if (capabilities & NOMMU_MAP_EXEC)
> - prot |= PROT_EXEC;
> - }
> } else if ((prot & PROT_READ) &&
> (prot & PROT_EXEC) &&
> !(capabilities & NOMMU_MAP_EXEC)
> @@ -810,11 +804,6 @@ static int validate_mmap_request(struct file *file,
> * privately mapped
> */
> capabilities = NOMMU_MAP_COPY;
> -
> - /* handle PROT_EXEC implication by PROT_READ */
> - if ((prot & PROT_READ) &&
> - (current->personality & READ_IMPLIES_EXEC))
> - prot |= PROT_EXEC;
> }
>
> /* allow the security API to have its say */
> @@ -992,6 +981,44 @@ static int do_mmap_private(struct vm_area_struct *vma,
> return -ENOMEM;
> }
>
> +int mmap_check_prot(struct file *file, unsigned long *prot,
> + unsigned long flags)
> +{
> + unsigned long req_prot = *prot;
> + unsigned long new_prot = req_prot;
> + int err;
> +
> + /*
> + * Does the application expect PROT_READ to imply PROT_EXEC?
> + *
> + * (the exception is when the underlying filesystem is noexec
> + * mounted or the file does not have NOMMU_MAP_EXEC
> + * (== VM_MAYEXEC), in which case we don't add PROT_EXEC.)
> + */
> + if ((req_prot & (PROT_READ | PROT_EXEC)) != PROT_READ)
> + goto check;
> + if (!(current->personality & READ_IMPLIES_EXEC))
> + goto check;
> + if (!file) {
> + new_prot |= PROT_EXEC;
> + goto check;
> + }
> + if (file->f_op->mmap_capabilities) {
> + unsigned int caps = file->f_op->mmap_capabilities(file);
> +
> + if (!(caps & NOMMU_MAP_EXEC))
> + goto check;
> + new_prot |= PROT_EXEC;
> + }
> +check:
> + err = security_mmap_file(file, req_prot, new_prot, flags);
> + if (err)
> + return err;
> +
> + *prot = new_prot;
> + return 0;
> +}
> +
> /*
> * handle mapping creation for uClinux
> */
> diff --git a/mm/util.c b/mm/util.c
> index bd283e2132e0..2eb4d6037610 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -581,7 +581,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
> unsigned long populate;
> LIST_HEAD(uf);
>
> - ret = security_mmap_file(file, prot, flag);
> + ret = mmap_check_prot(file, &prot, flag);
> if (!ret) {
> if (mmap_write_lock_killable(mm))
> return -EINTR;
> diff --git a/security/security.c b/security/security.c
> index 4564a0a1e4ef..25556629f588 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2927,42 +2927,10 @@ int security_file_ioctl_compat(struct file *file, unsigned int cmd,
> }
> EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
>
> -static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
> -{
> - /*
> - * Does we have PROT_READ and does the application expect
> - * it to imply PROT_EXEC? If not, nothing to talk about...
> - */
> - if ((prot & (PROT_READ | PROT_EXEC)) != PROT_READ)
> - return prot;
> - if (!(current->personality & READ_IMPLIES_EXEC))
> - return prot;
> - /*
> - * if that's an anonymous mapping, let it.
> - */
> - if (!file)
> - return prot | PROT_EXEC;
> - /*
> - * ditto if it's not on noexec mount, except that on !MMU we need
> - * NOMMU_MAP_EXEC (== VM_MAYEXEC) in this case
> - */
> - if (!path_noexec(&file->f_path)) {
> -#ifndef CONFIG_MMU
> - if (file->f_op->mmap_capabilities) {
> - unsigned caps = file->f_op->mmap_capabilities(file);
> - if (!(caps & NOMMU_MAP_EXEC))
> - return prot;
> - }
> -#endif
> - return prot | PROT_EXEC;
> - }
> - /* anything on noexec mount won't get PROT_EXEC */
> - return prot;
> -}
> -
> /**
> * security_mmap_file() - Check if mmap'ing a file is allowed
> * @file: file
> + * @reqprot: protection requested by user
> * @prot: protection applied by the kernel
> * @flags: flags
> *
> @@ -2971,11 +2939,10 @@ static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
> *
> * Return: Returns 0 if permission is granted.
> */
> -int security_mmap_file(struct file *file, unsigned long prot,
> - unsigned long flags)
> +int security_mmap_file(struct file *file, unsigned long reqprot,
> + unsigned long prot, unsigned long flags)
> {
> - return call_int_hook(mmap_file, file, prot, mmap_prot(file, prot),
> - flags);
> + return call_int_hook(mmap_file, file, reqprot, prot, flags);
> }
>
> /**
>
> base-commit: f89722faa31466ff41aed21bdeb9cf34c2312858
> --
> 2.34.1
>
> You have sent this non-RFC intentionally conflicting with [0] to provide > 'alternatives' that is not what a [PATCH] submission is. > > In any case, speculative changes like this should ABSOLUTELY be sent RFC, > and sending things that are merge conflicts as ordinary patches is actually > bordering on being a little rude! > > I'm sure it's unintentional :) but for the sake of us being able to work > with these properly you should just send one as RFC and ask whether it'd be > appropriate to send an alternative, and just allude to it in the one you do > send. > > [0]:https://lore.kernel.org/all/20240925081628.408-1-ebpqwerty472123@gmail.com/ I am very sorry that I sent the wrong subject which should add "RFC", due to lack of experience. > It's a bit weird to send 'alternatives' - you should by now have a good > sense of which ought to work, if not perhaps more research is required on > your part? I think both solutions can work, and the preliminary discussion is on the mail list for [1] (as this issue is related to security before it was fixed, the discussion is on security@kernel.org). The choice depends only on taste. Link: https://lore.kernel.org/all/20240919080905.4506-2-paul@paul-moore.com/ [1]
On Wed, Sep 25, 2024 at 05:09:47PM GMT, Shu Han wrote: > > You have sent this non-RFC intentionally conflicting with [0] to provide > > 'alternatives' that is not what a [PATCH] submission is. > > > > In any case, speculative changes like this should ABSOLUTELY be sent RFC, > > and sending things that are merge conflicts as ordinary patches is actually > > bordering on being a little rude! > > > > I'm sure it's unintentional :) but for the sake of us being able to work > > with these properly you should just send one as RFC and ask whether it'd be > > appropriate to send an alternative, and just allude to it in the one you do > > send. > > > > [0]:https://lore.kernel.org/all/20240925081628.408-1-ebpqwerty472123@gmail.com/ > > I am very sorry that I sent the wrong subject which should add "RFC", > due to lack of experience. No need to be sorry! :) Sorry if I sound harsh here - it's more a case of trying to be as clear as I can be as that is the best approach for everyone I think. This code is sensitive, so we have to super careful! > > > It's a bit weird to send 'alternatives' - you should by now have a good > > sense of which ought to work, if not perhaps more research is required on > > your part? > > I think both solutions can work, and the preliminary discussion is on > the mail list for [1] > (as this issue is related to security before it was fixed, the > discussion is on security@kernel.org). > The choice depends only on taste. > > Link: https://lore.kernel.org/all/20240919080905.4506-2-paul@paul-moore.com/ [1] I would disagree it's down to taste, I noted on the move the check to do_mmap() series a number of issues and concerns, to me that seems unworkable in it's current form, the locking thing is fatal for instance. What you link to there seems to be neither approach (I didn't read your second series though as that needs an RFC tag)? I mean I think perhaps what you are doing there is the best _first step_ - simply add the checks in each of the callsites that you feel are missing them. This is the least controversial way and then allows maintainers of the callers to assess whether they intended for that. If then you end up wtih _all_ callers doing this check, we can take another look at possibly bringing it into do_mmap() but we would absolutely have to ensure it was done correctly, however. I do feel we need to better document these functions, so I will add comments. I see you did so as part of your other series, but think maybe we need to expand this and possibly rename both and add some asserts... it's on the todo list! So moving forward I suggest: 1. (If you haven't already) Submit a series that adds patches to add checks at call sites that don't already check. 2. If these are accepted at _all_ callsites, revisit the do_mmap() change, properly accounting for locks (I can help with this). Thanks for your contribution!
> No need to be sorry! :) Sorry if I sound harsh here - it's more a case of > trying to be as clear as I can be as that is the best approach for everyone > I think. > This code is sensitive, so we have to super careful! Thanks a lot! :) > I would disagree it's down to taste, I noted on the move the check to > do_mmap() series a number of issues and concerns, to me that seems > unworkable in it's current form, the locking thing is fatal for instance. > What you link to there seems to be neither approach (I didn't read your > second series though as that needs an RFC tag)? I mean I think perhaps what > you are doing there is the best _first step_ - simply add the checks in > each of the callsites that you feel are missing them. > This is the least controversial way and then allows maintainers of the > callers to assess whether they intended for that. > If then you end up wtih _all_ callers doing this check, we can take another > look at possibly bringing it into do_mmap() but we would absolutely have to > ensure it was done correctly, however. > 1. (If you haven't already) Submit a series that adds patches to add checks > at call sites that don't already check. > 2. If these are accepted at _all_ callsites, revisit the do_mmap() change, > properly accounting for locks (I can help with this). In fact, "mm: move the check of READ_IMPLIES_EXEC out of do_mmap()" does not have the locking issue. These two patches are quite different. This is also the modification I recommended, while another modification was suggested by LSM maintainers(Perhaps I need to add suggested-by? But that was mentioned in a non-public security mailing list, and I'm not sure if it's appropriate.). The __core__ problem is "no LSM hook" + "have logic about READ_IMPLIES_EXEC". Removing one of them is OK. The "mm: move security_file_mmap() back into do_mmap()" fixes this by adding a LSM hook. The requirement to call LSM hooks comes from the LSM modules, _not these call sites_. The issue for locks also comes from the specific implementation of LSM modules. So I send patches to LSM maintainers at the same time. The "mm: move the check of READ_IMPLIES_EXEC out of do_mmap()" fixes this by removing the logic about READ_IMPLIES_EXEC that is not needed. So no locking issues there(no changes to LSM). This will result in some minor behavioral changes for call sites mentioned in the patch. Unfortunately, due to this logic being placed in a single function do_mmap now, it is impossible to confirm it through patches one by one before change the mm module. Fortunately, these changes should clearly be fine, and here are the reasons(more specific versions): fs/aio.c, mm/util.c, ipc/shm.c: no changes arch/x86/kernel/shstk.c: Shadow Stack is stack only store return addresses, adding execute permission to shadow stack is never required. mm/mmap.c: in the history, remap_file_pages won't care about the READ_IMPLIES_EXEC. this side effect is introduced in the emulated version, after the deprecated mark exists. The patch only removes the side effects introduced. And this(mm) is the module. :) BTW, The link is the _first step_ in required(if the check is missing in that call site, there will be a bug) call sites, which has been done. > I do feel we need to better document these functions, so I will add > comments. I see you did so as part of your other series, but think maybe we > need to expand this and possibly rename both and add some asserts... it's > on the todo list! Perhaps adding sufficient comments is also a completely appropriate method as another alternative. Thanks for your kind review!
© 2016 - 2024 Red Hat, Inc.