linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jann Horn <jannh@google.com>
To: Salvatore Mesoraca <s.mesoraca16@gmail.com>
Cc: kernel list <linux-kernel@vger.kernel.org>,
	Kernel Hardening <kernel-hardening@lists.openwall.com>,
	Linux-MM <linux-mm@kvack.org>,
	linux-security-module <linux-security-module@vger.kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Brad Spengler <spender@grsecurity.net>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Christoph Hellwig <hch@infradead.org>,
	James Morris <james.l.morris@oracle.com>,
	Kees Cook <keescook@chromium.org>,
	PaX Team <pageexec@freemail.hu>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v5 11/12] S.A.R.A.: /proc/*/mem write limitation
Date: Sat, 6 Jul 2019 20:20:35 +0200	[thread overview]
Message-ID: <CAG48ez0uFX4AniOk1W0Vs6j=7Q5QfSFQTrBBzC2qL2bpWn_yCg@mail.gmail.com> (raw)
In-Reply-To: <1562410493-8661-12-git-send-email-s.mesoraca16@gmail.com>

On Sat, Jul 6, 2019 at 12:55 PM Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> A process could use it to overwrite read-only memory, bypassing
> S.A.R.A. restrictions.
[...]
> +static void sara_task_to_inode(struct task_struct *t, struct inode *i)
> +{
> +       get_sara_inode_task(i) = t;

This looks bogus. Nothing is actually holding a reference to `t` here, right?

> +}
> +
>  static struct security_hook_list data_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(cred_prepare, sara_cred_prepare),
>         LSM_HOOK_INIT(cred_transfer, sara_cred_transfer),
>         LSM_HOOK_INIT(shm_alloc_security, sara_shm_alloc_security),
> +       LSM_HOOK_INIT(task_to_inode, sara_task_to_inode),
>  };
[...]
> +static int sara_file_open(struct file *file)
> +{
> +       struct task_struct *t;
> +       struct mm_struct *mm;
> +       u16 sara_wxp_flags = get_current_sara_wxp_flags();
> +
> +       /*
> +        * Prevent write access to /proc/.../mem
> +        * if it operates on the mm_struct of the
> +        * current process: it could be used to
> +        * bypass W^X.
> +        */
> +
> +       if (!sara_enabled ||
> +           !wxprot_enabled ||
> +           !(sara_wxp_flags & SARA_WXP_WXORX) ||
> +           !(file->f_mode & FMODE_WRITE))
> +               return 0;
> +
> +       t = get_sara_inode_task(file_inode(file));
> +       if (unlikely(t != NULL &&
> +                    strcmp(file->f_path.dentry->d_name.name,
> +                           "mem") == 0)) {

This should probably at least have a READ_ONCE() somewhere in case the
file concurrently gets renamed?

> +               get_task_struct(t);
> +               mm = get_task_mm(t);
> +               put_task_struct(t);

Getting and dropping a reference to the task_struct here is completely
useless. Either you have a reference, in which case you don't need to
take another one, or you don't have a reference, in which case you
also can't take one.

> +               if (unlikely(mm == current->mm))
> +                       sara_warn_or_goto(error,
> +                                         "write access to /proc/*/mem");

Why is the current process so special that it must be protected more
than other processes? Is the idea here to rely on other protections to
protect all other tasks? This should probably come with a comment that
explains this choice.

> +               mmput(mm);
> +       }
> +       return 0;
> +error:
> +       mmput(mm);
> +       return -EACCES;
> +}

  reply	other threads:[~2019-07-06 18:21 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-06 10:54 [PATCH v5 00/12] S.A.R.A. a new stacked LSM Salvatore Mesoraca
2019-07-06 10:54 ` [PATCH v5 01/12] S.A.R.A.: add documentation Salvatore Mesoraca
2019-07-06 17:14   ` Randy Dunlap
2019-07-06 17:32     ` Salvatore Mesoraca
2019-07-13  0:14   ` James Morris
2019-07-06 10:54 ` [PATCH v5 02/12] S.A.R.A.: create framework Salvatore Mesoraca
2019-07-06 15:29   ` Randy Dunlap
2019-07-06 10:54 ` [PATCH v5 03/12] S.A.R.A.: cred blob management Salvatore Mesoraca
2019-07-12 23:35   ` James Morris
2019-07-06 10:54 ` [PATCH v5 04/12] S.A.R.A.: generic DFA for string matching Salvatore Mesoraca
2019-07-06 18:32   ` Jann Horn
2019-07-07 16:01     ` Salvatore Mesoraca
2019-07-08 17:37       ` Jann Horn
2019-10-06 16:49       ` Salvatore Mesoraca
2019-10-07 12:40         ` Jann Horn
2019-07-06 10:54 ` [PATCH v5 05/12] LSM: creation of "check_vmflags" LSM hook Salvatore Mesoraca
2019-07-06 10:54 ` [PATCH v5 06/12] S.A.R.A.: WX protection Salvatore Mesoraca
2019-07-06 15:38   ` Randy Dunlap
2019-07-06 19:28   ` Al Viro
2019-07-07 15:49     ` Salvatore Mesoraca
2019-07-09  4:51       ` Kees Cook
2019-07-08 12:42   ` David Laight
2019-07-06 10:54 ` [PATCH v5 07/12] LSM: creation of "pagefault_handler" LSM hook Salvatore Mesoraca
2019-07-06 10:54 ` [PATCH v5 08/12] S.A.R.A.: trampoline emulation Salvatore Mesoraca
2019-07-06 15:31   ` Randy Dunlap
2019-07-06 10:54 ` [PATCH v5 09/12] S.A.R.A.: WX protection procattr interface Salvatore Mesoraca
2019-07-06 10:54 ` [PATCH v5 10/12] S.A.R.A.: XATTRs support Salvatore Mesoraca
2019-07-06 10:54 ` [PATCH v5 11/12] S.A.R.A.: /proc/*/mem write limitation Salvatore Mesoraca
2019-07-06 18:20   ` Jann Horn [this message]
2019-07-07 16:15     ` Salvatore Mesoraca
2019-07-06 10:54 ` [PATCH v5 12/12] MAINTAINERS: take maintainership for S.A.R.A Salvatore Mesoraca
2019-07-06 14:33 ` [PATCH v5 00/12] S.A.R.A. a new stacked LSM Jordan Glover
2019-07-06 15:02   ` Salvatore Mesoraca
2019-07-07  1:16 ` James Morris
2019-07-07 15:40   ` Salvatore Mesoraca

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAG48ez0uFX4AniOk1W0Vs6j=7Q5QfSFQTrBBzC2qL2bpWn_yCg@mail.gmail.com' \
    --to=jannh@google.com \
    --cc=casey@schaufler-ca.com \
    --cc=hch@infradead.org \
    --cc=james.l.morris@oracle.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=pageexec@freemail.hu \
    --cc=s.mesoraca16@gmail.com \
    --cc=serge@hallyn.com \
    --cc=spender@grsecurity.net \
    --cc=tglx@linutronix.de \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v5 11/12] S.A.R.A.: /proc/*/mem write limitation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).