Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Alexey Dobriyan]

On Wed, Nov 29, 2023 at 05:11:22PM -0800, Andrew Morton wrote:
> 
> fyi...
> 
> (yuk!)
> 
> 
> 
> Begin forwarded message:
> 
> Date: Thu, 30 Nov 2023 00:37:04 +0000
> From: Munehisa Kamata <kamatam@amazon.com>
> To: <linux-fsdevel@vger.kernel.org>, <linux-security-module@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>, <akpm@linux-foundation.org>, "Munehisa Kamata" <kamatam@amazon.com>
> Subject: [PATCH] proc: Update inode upon changing task security attribute
> 
> 
> I'm not clear whether VFS is a better (or worse) place[1] to fix the
> problem described below and would like to hear opinion.
> 
> If the /proc/[pid] directory is bind-mounted on a system with Smack
> enabled, and if the task updates its current security attribute, the task
> may lose access to files in its own /proc/[pid] through the mountpoint.
> 
>  $ sudo capsh --drop=cap_mac_override --
>  # mkdir -p dir
>  # mount --bind /proc/$$ dir
>  # echo AAA > /proc/$$/task/current		# assuming built-in echo
>  # cat /proc/$$/task/current			# revalidate
>  AAA
>  # echo BBB > dir/attr/current
>  # cat dir/attr/current
>  cat: dir/attr/current: Permission denied
>  # ls dir/
>  ls: cannot access dir/: Permission denied
>  # cat /proc/$$/attr/current			# revalidate
>  BBB
>  # cat dir/attr/current
>  BBB
>  # echo CCC > /proc/$$/attr/current
>  # cat dir/attr/current
>  cat: dir/attr/current: Permission denied
> 
> This happens because path lookup doesn't revalidate the dentry of the
> /proc/[pid] when traversing the filesystem boundary, so the inode security
> blob of the /proc/[pid] doesn't get updated with the new task security
> attribute. Then, this may lead security modules to deny an access to the
> directory. Looking at the code[2] and the /proc/pid/attr/current entry in
> proc man page, seems like the same could happen with SELinux. Though, I
> didn't find relevant reports.
> 
> The steps above are quite artificial. I actually encountered such an
> unexpected denial of access with an in-house application sandbox
> framework; each app has its own dedicated filesystem tree where the
> process's /proc/[pid] is bind-mounted to and the app enters into via
> chroot.
> 
> With this patch, writing to /proc/[pid]/attr/current (and its per-security
> module variant) updates the inode security blob of /proc/[pid] or
> /proc/[pid]/task/[tid] (when pid != tid) with the new attribute.
> 
> [1] https://lkml.kernel.org/linux-fsdevel/4A2D15AF.8090000@sun.com/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/selinux/hooks.c#n4220
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> ---
>  fs/proc/base.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index dd31e3b6bf77..bdb7bea53475 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2741,6 +2741,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  {
>  	struct inode * inode = file_inode(file);
>  	struct task_struct *task;
> +	const char *name = file->f_path.dentry->d_name.name;
>  	void *page;
>  	int rv;
>  
> @@ -2784,10 +2785,26 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  	if (rv < 0)
>  		goto out_free;
>  
> -	rv = security_setprocattr(PROC_I(inode)->op.lsm,
> -				  file->f_path.dentry->d_name.name, page,
> -				  count);
> +	rv = security_setprocattr(PROC_I(inode)->op.lsm, name, page, count);
>  	mutex_unlock(&current->signal->cred_guard_mutex);
> +
> +	/*
> +	 *  Update the inode security blob in advance if the task's security
> +	 *  attribute was updated
> +	 */
> +	if (rv > 0 && !strcmp(name, "current")) {
> +		struct pid *pid;
> +		struct proc_inode *cur, *ei;
> +
> +		rcu_read_lock();
> +		pid = get_task_pid(current, PIDTYPE_PID);
> +		hlist_for_each_entry(cur, &pid->inodes, sibling_inodes)
> +			ei = cur;

Should this "break;"? Why is only the last inode in the list updated?
Should it be the first? All of them?

> +		put_pid(pid);
> +		pid_update_inode(current, &ei->vfs_inode);
> +		rcu_read_unlock();
> +	}

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Munehisa Kamata]

Hi Alexey,

On Fri, 2023-12-01 09:30:00 +0000, Alexey Dobriyan wrote:
>
> On Wed, Nov 29, 2023 at 05:11:22PM -0800, Andrew Morton wrote:
> > 
> > fyi...
> > 
> > (yuk!)
> > 
> > 
> > 
> > Begin forwarded message:
> > 
> > Date: Thu, 30 Nov 2023 00:37:04 +0000
> > From: Munehisa Kamata <kamatam@amazon.com>
> > To: <linux-fsdevel@vger.kernel.org>, <linux-security-module@vger.kernel.org>
> > Cc: <linux-kernel@vger.kernel.org>, <akpm@linux-foundation.org>, "Munehisa Kamata" <kamatam@amazon.com>
> > Subject: [PATCH] proc: Update inode upon changing task security attribute
> > 
> > 
> > I'm not clear whether VFS is a better (or worse) place[1] to fix the
> > problem described below and would like to hear opinion.
> > 
> > If the /proc/[pid] directory is bind-mounted on a system with Smack
> > enabled, and if the task updates its current security attribute, the task
> > may lose access to files in its own /proc/[pid] through the mountpoint.
> > 
> >  $ sudo capsh --drop=cap_mac_override --
> >  # mkdir -p dir
> >  # mount --bind /proc/$$ dir
> >  # echo AAA > /proc/$$/task/current		# assuming built-in echo
> >  # cat /proc/$$/task/current			# revalidate
> >  AAA
> >  # echo BBB > dir/attr/current
> >  # cat dir/attr/current
> >  cat: dir/attr/current: Permission denied
> >  # ls dir/
> >  ls: cannot access dir/: Permission denied
> >  # cat /proc/$$/attr/current			# revalidate
> >  BBB
> >  # cat dir/attr/current
> >  BBB
> >  # echo CCC > /proc/$$/attr/current
> >  # cat dir/attr/current
> >  cat: dir/attr/current: Permission denied
> > 
> > This happens because path lookup doesn't revalidate the dentry of the
> > /proc/[pid] when traversing the filesystem boundary, so the inode security
> > blob of the /proc/[pid] doesn't get updated with the new task security
> > attribute. Then, this may lead security modules to deny an access to the
> > directory. Looking at the code[2] and the /proc/pid/attr/current entry in
> > proc man page, seems like the same could happen with SELinux. Though, I
> > didn't find relevant reports.
> > 
> > The steps above are quite artificial. I actually encountered such an
> > unexpected denial of access with an in-house application sandbox
> > framework; each app has its own dedicated filesystem tree where the
> > process's /proc/[pid] is bind-mounted to and the app enters into via
> > chroot.
> > 
> > With this patch, writing to /proc/[pid]/attr/current (and its per-security
> > module variant) updates the inode security blob of /proc/[pid] or
> > /proc/[pid]/task/[tid] (when pid != tid) with the new attribute.
> > 
> > [1] https://lkml.kernel.org/linux-fsdevel/4A2D15AF.8090000@sun.com/
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/selinux/hooks.c#n4220
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> > ---
> >  fs/proc/base.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index dd31e3b6bf77..bdb7bea53475 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -2741,6 +2741,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> >  {
> >  	struct inode * inode = file_inode(file);
> >  	struct task_struct *task;
> > +	const char *name = file->f_path.dentry->d_name.name;
> >  	void *page;
> >  	int rv;
> >  
> > @@ -2784,10 +2785,26 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> >  	if (rv < 0)
> >  		goto out_free;
> >  
> > -	rv = security_setprocattr(PROC_I(inode)->op.lsm,
> > -				  file->f_path.dentry->d_name.name, page,
> > -				  count);
> > +	rv = security_setprocattr(PROC_I(inode)->op.lsm, name, page, count);
> >  	mutex_unlock(&current->signal->cred_guard_mutex);
> > +
> > +	/*
> > +	 *  Update the inode security blob in advance if the task's security
> > +	 *  attribute was updated
> > +	 */
> > +	if (rv > 0 && !strcmp(name, "current")) {
> > +		struct pid *pid;
> > +		struct proc_inode *cur, *ei;
> > +
> > +		rcu_read_lock();
> > +		pid = get_task_pid(current, PIDTYPE_PID);
> > +		hlist_for_each_entry(cur, &pid->inodes, sibling_inodes)
> > +			ei = cur;
> 
> Should this "break;"? Why is only the last inode in the list updated?
> Should it be the first? All of them?

If it picks up the first node, it may end up updating /proc/[pid]/task/[tid]
rather than /proc/[pid] (when pid == tid) and the task may be denied access
to its own /proc/[pid] afterward.

I think updating all of them won't hurt. But, as long as /proc/[pid] is
accessible, the rest of the inodes should be updated upon path lookup via
revalidation as usual.

When pid != tid, it only updates /proc/[pid]/task/[tid] and the thread may
lose an access to /proc/[pid], but I think it's okay as it's a matter of
security policy enforced by security modules. Casey, do you have any
comments here?  


Regards,
Munehisa

 
> > +		put_pid(pid);
> > +		pid_update_inode(current, &ei->vfs_inode);
> > +		rcu_read_unlock();
> > +	}
> 

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Casey Schaufler]

On 12/1/2023 12:59 PM, Munehisa Kamata wrote:
> Hi Alexey,
>
> On Fri, 2023-12-01 09:30:00 +0000, Alexey Dobriyan wrote:
>> On Wed, Nov 29, 2023 at 05:11:22PM -0800, Andrew Morton wrote:
>>> fyi...
>>>
>>> (yuk!)
>>>
>>>
>>>
>>> Begin forwarded message:
>>>
>>> Date: Thu, 30 Nov 2023 00:37:04 +0000
>>> From: Munehisa Kamata <kamatam@amazon.com>
>>> To: <linux-fsdevel@vger.kernel.org>, <linux-security-module@vger.kernel.org>
>>> Cc: <linux-kernel@vger.kernel.org>, <akpm@linux-foundation.org>, "Munehisa Kamata" <kamatam@amazon.com>
>>> Subject: [PATCH] proc: Update inode upon changing task security attribute
>>>
>>>
>>> I'm not clear whether VFS is a better (or worse) place[1] to fix the
>>> problem described below and would like to hear opinion.
>>>
>>> If the /proc/[pid] directory is bind-mounted on a system with Smack
>>> enabled, and if the task updates its current security attribute, the task
>>> may lose access to files in its own /proc/[pid] through the mountpoint.
>>>
>>>  $ sudo capsh --drop=cap_mac_override --
>>>  # mkdir -p dir
>>>  # mount --bind /proc/$$ dir
>>>  # echo AAA > /proc/$$/task/current		# assuming built-in echo
>>>  # cat /proc/$$/task/current			# revalidate
>>>  AAA
>>>  # echo BBB > dir/attr/current
>>>  # cat dir/attr/current
>>>  cat: dir/attr/current: Permission denied
>>>  # ls dir/
>>>  ls: cannot access dir/: Permission denied
>>>  # cat /proc/$$/attr/current			# revalidate
>>>  BBB
>>>  # cat dir/attr/current
>>>  BBB
>>>  # echo CCC > /proc/$$/attr/current
>>>  # cat dir/attr/current
>>>  cat: dir/attr/current: Permission denied
>>>
>>> This happens because path lookup doesn't revalidate the dentry of the
>>> /proc/[pid] when traversing the filesystem boundary, so the inode security
>>> blob of the /proc/[pid] doesn't get updated with the new task security
>>> attribute. Then, this may lead security modules to deny an access to the
>>> directory. Looking at the code[2] and the /proc/pid/attr/current entry in
>>> proc man page, seems like the same could happen with SELinux. Though, I
>>> didn't find relevant reports.
>>>
>>> The steps above are quite artificial. I actually encountered such an
>>> unexpected denial of access with an in-house application sandbox
>>> framework; each app has its own dedicated filesystem tree where the
>>> process's /proc/[pid] is bind-mounted to and the app enters into via
>>> chroot.
>>>
>>> With this patch, writing to /proc/[pid]/attr/current (and its per-security
>>> module variant) updates the inode security blob of /proc/[pid] or
>>> /proc/[pid]/task/[tid] (when pid != tid) with the new attribute.
>>>
>>> [1] https://lkml.kernel.org/linux-fsdevel/4A2D15AF.8090000@sun.com/
>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/selinux/hooks.c#n4220
>>>
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
>>> ---
>>>  fs/proc/base.c | 23 ++++++++++++++++++++---
>>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index dd31e3b6bf77..bdb7bea53475 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -2741,6 +2741,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>>>  {
>>>  	struct inode * inode = file_inode(file);
>>>  	struct task_struct *task;
>>> +	const char *name = file->f_path.dentry->d_name.name;
>>>  	void *page;
>>>  	int rv;
>>>  
>>> @@ -2784,10 +2785,26 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>>>  	if (rv < 0)
>>>  		goto out_free;
>>>  
>>> -	rv = security_setprocattr(PROC_I(inode)->op.lsm,
>>> -				  file->f_path.dentry->d_name.name, page,
>>> -				  count);
>>> +	rv = security_setprocattr(PROC_I(inode)->op.lsm, name, page, count);
>>>  	mutex_unlock(&current->signal->cred_guard_mutex);
>>> +
>>> +	/*
>>> +	 *  Update the inode security blob in advance if the task's security
>>> +	 *  attribute was updated
>>> +	 */
>>> +	if (rv > 0 && !strcmp(name, "current")) {
>>> +		struct pid *pid;
>>> +		struct proc_inode *cur, *ei;
>>> +
>>> +		rcu_read_lock();
>>> +		pid = get_task_pid(current, PIDTYPE_PID);
>>> +		hlist_for_each_entry(cur, &pid->inodes, sibling_inodes)
>>> +			ei = cur;
>> Should this "break;"? Why is only the last inode in the list updated?
>> Should it be the first? All of them?
> If it picks up the first node, it may end up updating /proc/[pid]/task/[tid]
> rather than /proc/[pid] (when pid == tid) and the task may be denied access
> to its own /proc/[pid] afterward.
>
> I think updating all of them won't hurt. But, as long as /proc/[pid] is
> accessible, the rest of the inodes should be updated upon path lookup via
> revalidation as usual.
>
> When pid != tid, it only updates /proc/[pid]/task/[tid] and the thread may
> lose an access to /proc/[pid], but I think it's okay as it's a matter of
> security policy enforced by security modules. Casey, do you have any
> comments here?

I do not.

>
>
> Regards,
> Munehisa
>
>  
>>> +		put_pid(pid);
>>> +		pid_update_inode(current, &ei->vfs_inode);
>>> +		rcu_read_unlock();
>>> +	}

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Paul Moore]

On Fri, Dec 1, 2023 at 4:00 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> On Fri, 2023-12-01 09:30:00 +0000, Alexey Dobriyan wrote:
> > On Wed, Nov 29, 2023 at 05:11:22PM -0800, Andrew Morton wrote:
> > >
> > > fyi...
> > >
> > > (yuk!)
> > >
> > > Begin forwarded message:
> > > Date: Thu, 30 Nov 2023 00:37:04 +0000
> > > From: Munehisa Kamata <kamatam@amazon.com>
> > > Subject: [PATCH] proc: Update inode upon changing task security attribute
> > >
> > > I'm not clear whether VFS is a better (or worse) place[1] to fix the
> > > problem described below and would like to hear opinion.
> > >
> > > If the /proc/[pid] directory is bind-mounted on a system with Smack
> > > enabled, and if the task updates its current security attribute, the task
> > > may lose access to files in its own /proc/[pid] through the mountpoint.
> > >
> > >  $ sudo capsh --drop=cap_mac_override --
> > >  # mkdir -p dir
> > >  # mount --bind /proc/$$ dir
> > >  # echo AAA > /proc/$$/task/current         # assuming built-in echo
> > >  # cat /proc/$$/task/current                        # revalidate
> > >  AAA
> > >  # echo BBB > dir/attr/current
> > >  # cat dir/attr/current
> > >  cat: dir/attr/current: Permission denied
> > >  # ls dir/
> > >  ls: cannot access dir/: Permission denied
> > >  # cat /proc/$$/attr/current                        # revalidate
> > >  BBB
> > >  # cat dir/attr/current
> > >  BBB
> > >  # echo CCC > /proc/$$/attr/current
> > >  # cat dir/attr/current
> > >  cat: dir/attr/current: Permission denied
> > >
> > > This happens because path lookup doesn't revalidate the dentry of the
> > > /proc/[pid] when traversing the filesystem boundary, so the inode security
> > > blob of the /proc/[pid] doesn't get updated with the new task security
> > > attribute. Then, this may lead security modules to deny an access to the
> > > directory. Looking at the code[2] and the /proc/pid/attr/current entry in
> > > proc man page, seems like the same could happen with SELinux. Though, I
> > > didn't find relevant reports.
> > >
> > > The steps above are quite artificial. I actually encountered such an
> > > unexpected denial of access with an in-house application sandbox
> > > framework; each app has its own dedicated filesystem tree where the
> > > process's /proc/[pid] is bind-mounted to and the app enters into via
> > > chroot.
> > >
> > > With this patch, writing to /proc/[pid]/attr/current (and its per-security
> > > module variant) updates the inode security blob of /proc/[pid] or
> > > /proc/[pid]/task/[tid] (when pid != tid) with the new attribute.
> > >
> > > [1] https://lkml.kernel.org/linux-fsdevel/4A2D15AF.8090000@sun.com/
> > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/selinux/hooks.c#n4220
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> > > ---
> > >  fs/proc/base.c | 23 ++++++++++++++++++++---
> > >  1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index dd31e3b6bf77..bdb7bea53475 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -2741,6 +2741,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> > >  {
> > >     struct inode * inode = file_inode(file);
> > >     struct task_struct *task;
> > > +   const char *name = file->f_path.dentry->d_name.name;
> > >     void *page;
> > >     int rv;
> > >
> > > @@ -2784,10 +2785,26 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> > >     if (rv < 0)
> > >             goto out_free;
> > >
> > > -   rv = security_setprocattr(PROC_I(inode)->op.lsm,
> > > -                             file->f_path.dentry->d_name.name, page,
> > > -                             count);
> > > +   rv = security_setprocattr(PROC_I(inode)->op.lsm, name, page, count);
> > >     mutex_unlock(&current->signal->cred_guard_mutex);
> > > +
> > > +   /*
> > > +    *  Update the inode security blob in advance if the task's security
> > > +    *  attribute was updated
> > > +    */
> > > +   if (rv > 0 && !strcmp(name, "current")) {
> > > +           struct pid *pid;
> > > +           struct proc_inode *cur, *ei;
> > > +
> > > +           rcu_read_lock();
> > > +           pid = get_task_pid(current, PIDTYPE_PID);
> > > +           hlist_for_each_entry(cur, &pid->inodes, sibling_inodes)
> > > +                   ei = cur;
> >
> > Should this "break;"? Why is only the last inode in the list updated?
> > Should it be the first? All of them?
>
> If it picks up the first node, it may end up updating /proc/[pid]/task/[tid]
> rather than /proc/[pid] (when pid == tid) and the task may be denied access
> to its own /proc/[pid] afterward.
>
> I think updating all of them won't hurt. But, as long as /proc/[pid] is
> accessible, the rest of the inodes should be updated upon path lookup via
> revalidation as usual.
>
> When pid != tid, it only updates /proc/[pid]/task/[tid] and the thread may
> lose an access to /proc/[pid], but I think it's okay as it's a matter of
> security policy enforced by security modules. Casey, do you have any
> comments here?
>
> > > +           put_pid(pid);
> > > +           pid_update_inode(current, &ei->vfs_inode);
> > > +           rcu_read_unlock();
> > > +   }

I think my thoughts are neatly summarized by Andrew's "yuk!" comment
at the top.  However, before we go too much further on this, can we
get clarification that Casey was able to reproduce this on a stock
upstream kernel?  Last I read in the other thread Casey wasn't seeing
this problem on Linux v6.5.

However, for the moment I'm going to assume this is a real problem, is
there some reason why the existing pid_revalidate() code is not being
called in the bind mount case?  From what I can see in the original
problem report, the path walk seems to work okay when the file is
accessed directly from /proc, but fails when done on the bind mount.
Is there some problem with revalidating dentrys on bind mounts?

-- 
paul-moore.com

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Casey Schaufler]

On 12/5/2023 2:21 PM, Paul Moore wrote:
> On Fri, Dec 1, 2023 at 4:00 PM Munehisa Kamata <kamatam@amazon.com> wrote:
>> On Fri, 2023-12-01 09:30:00 +0000, Alexey Dobriyan wrote:
>>> On Wed, Nov 29, 2023 at 05:11:22PM -0800, Andrew Morton wrote:
>>>> fyi...
>>>>
>>>> (yuk!)
>>>>
>>>> Begin forwarded message:
>>>> Date: Thu, 30 Nov 2023 00:37:04 +0000
>>>> From: Munehisa Kamata <kamatam@amazon.com>
>>>> Subject: [PATCH] proc: Update inode upon changing task security attribute
>>>>
>>>> I'm not clear whether VFS is a better (or worse) place[1] to fix the
>>>> problem described below and would like to hear opinion.
>>>>
>>>> If the /proc/[pid] directory is bind-mounted on a system with Smack
>>>> enabled, and if the task updates its current security attribute, the task
>>>> may lose access to files in its own /proc/[pid] through the mountpoint.
>>>>
>>>>  $ sudo capsh --drop=cap_mac_override --
>>>>  # mkdir -p dir
>>>>  # mount --bind /proc/$$ dir
>>>>  # echo AAA > /proc/$$/task/current         # assuming built-in echo
>>>>  # cat /proc/$$/task/current                        # revalidate
>>>>  AAA
>>>>  # echo BBB > dir/attr/current
>>>>  # cat dir/attr/current
>>>>  cat: dir/attr/current: Permission denied
>>>>  # ls dir/
>>>>  ls: cannot access dir/: Permission denied
>>>>  # cat /proc/$$/attr/current                        # revalidate
>>>>  BBB
>>>>  # cat dir/attr/current
>>>>  BBB
>>>>  # echo CCC > /proc/$$/attr/current
>>>>  # cat dir/attr/current
>>>>  cat: dir/attr/current: Permission denied
>>>>
>>>> This happens because path lookup doesn't revalidate the dentry of the
>>>> /proc/[pid] when traversing the filesystem boundary, so the inode security
>>>> blob of the /proc/[pid] doesn't get updated with the new task security
>>>> attribute. Then, this may lead security modules to deny an access to the
>>>> directory. Looking at the code[2] and the /proc/pid/attr/current entry in
>>>> proc man page, seems like the same could happen with SELinux. Though, I
>>>> didn't find relevant reports.
>>>>
>>>> The steps above are quite artificial. I actually encountered such an
>>>> unexpected denial of access with an in-house application sandbox
>>>> framework; each app has its own dedicated filesystem tree where the
>>>> process's /proc/[pid] is bind-mounted to and the app enters into via
>>>> chroot.
>>>>
>>>> With this patch, writing to /proc/[pid]/attr/current (and its per-security
>>>> module variant) updates the inode security blob of /proc/[pid] or
>>>> /proc/[pid]/task/[tid] (when pid != tid) with the new attribute.
>>>>
>>>> [1] https://lkml.kernel.org/linux-fsdevel/4A2D15AF.8090000@sun.com/
>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/selinux/hooks.c#n4220
>>>>
>>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>>> Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
>>>> ---
>>>>  fs/proc/base.c | 23 ++++++++++++++++++++---
>>>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>>> index dd31e3b6bf77..bdb7bea53475 100644
>>>> --- a/fs/proc/base.c
>>>> +++ b/fs/proc/base.c
>>>> @@ -2741,6 +2741,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>>>>  {
>>>>     struct inode * inode = file_inode(file);
>>>>     struct task_struct *task;
>>>> +   const char *name = file->f_path.dentry->d_name.name;
>>>>     void *page;
>>>>     int rv;
>>>>
>>>> @@ -2784,10 +2785,26 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>>>>     if (rv < 0)
>>>>             goto out_free;
>>>>
>>>> -   rv = security_setprocattr(PROC_I(inode)->op.lsm,
>>>> -                             file->f_path.dentry->d_name.name, page,
>>>> -                             count);
>>>> +   rv = security_setprocattr(PROC_I(inode)->op.lsm, name, page, count);
>>>>     mutex_unlock(&current->signal->cred_guard_mutex);
>>>> +
>>>> +   /*
>>>> +    *  Update the inode security blob in advance if the task's security
>>>> +    *  attribute was updated
>>>> +    */
>>>> +   if (rv > 0 && !strcmp(name, "current")) {
>>>> +           struct pid *pid;
>>>> +           struct proc_inode *cur, *ei;
>>>> +
>>>> +           rcu_read_lock();
>>>> +           pid = get_task_pid(current, PIDTYPE_PID);
>>>> +           hlist_for_each_entry(cur, &pid->inodes, sibling_inodes)
>>>> +                   ei = cur;
>>> Should this "break;"? Why is only the last inode in the list updated?
>>> Should it be the first? All of them?
>> If it picks up the first node, it may end up updating /proc/[pid]/task/[tid]
>> rather than /proc/[pid] (when pid == tid) and the task may be denied access
>> to its own /proc/[pid] afterward.
>>
>> I think updating all of them won't hurt. But, as long as /proc/[pid] is
>> accessible, the rest of the inodes should be updated upon path lookup via
>> revalidation as usual.
>>
>> When pid != tid, it only updates /proc/[pid]/task/[tid] and the thread may
>> lose an access to /proc/[pid], but I think it's okay as it's a matter of
>> security policy enforced by security modules. Casey, do you have any
>> comments here?
>>
>>>> +           put_pid(pid);
>>>> +           pid_update_inode(current, &ei->vfs_inode);
>>>> +           rcu_read_unlock();
>>>> +   }
> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> at the top.  However, before we go too much further on this, can we
> get clarification that Casey was able to reproduce this on a stock
> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> this problem on Linux v6.5.

I was able to recreate the problem once given corrected instructions,
which have to be followed exactly. 

> However, for the moment I'm going to assume this is a real problem, is
> there some reason why the existing pid_revalidate() code is not being
> called in the bind mount case?  From what I can see in the original
> problem report, the path walk seems to work okay when the file is
> accessed directly from /proc, but fails when done on the bind mount.
> Is there some problem with revalidating dentrys on bind mounts?
>

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Munehisa Kamata]

On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
>
> On Fri, Dec 1, 2023 at 4:00 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > On Fri, 2023-12-01 09:30:00 +0000, Alexey Dobriyan wrote:
> > > On Wed, Nov 29, 2023 at 05:11:22PM -0800, Andrew Morton wrote:
> > > >
> > > > fyi...
> > > >
> > > > (yuk!)
> > > >
> > > > Begin forwarded message:
> > > > Date: Thu, 30 Nov 2023 00:37:04 +0000
> > > > From: Munehisa Kamata <kamatam@amazon.com>
> > > > Subject: [PATCH] proc: Update inode upon changing task security attribute
> > > >
> > > > I'm not clear whether VFS is a better (or worse) place[1] to fix the
> > > > problem described below and would like to hear opinion.
> > > >
> > > > If the /proc/[pid] directory is bind-mounted on a system with Smack
> > > > enabled, and if the task updates its current security attribute, the task
> > > > may lose access to files in its own /proc/[pid] through the mountpoint.
> > > >
> > > >  $ sudo capsh --drop=cap_mac_override --
> > > >  # mkdir -p dir
> > > >  # mount --bind /proc/$$ dir
> > > >  # echo AAA > /proc/$$/task/current         # assuming built-in echo
> > > >  # cat /proc/$$/task/current                        # revalidate
> > > >  AAA
> > > >  # echo BBB > dir/attr/current
> > > >  # cat dir/attr/current
> > > >  cat: dir/attr/current: Permission denied
> > > >  # ls dir/
> > > >  ls: cannot access dir/: Permission denied
> > > >  # cat /proc/$$/attr/current                        # revalidate
> > > >  BBB
> > > >  # cat dir/attr/current
> > > >  BBB
> > > >  # echo CCC > /proc/$$/attr/current
> > > >  # cat dir/attr/current
> > > >  cat: dir/attr/current: Permission denied
> > > >
> > > > This happens because path lookup doesn't revalidate the dentry of the
> > > > /proc/[pid] when traversing the filesystem boundary, so the inode security
> > > > blob of the /proc/[pid] doesn't get updated with the new task security
> > > > attribute. Then, this may lead security modules to deny an access to the
> > > > directory. Looking at the code[2] and the /proc/pid/attr/current entry in
> > > > proc man page, seems like the same could happen with SELinux. Though, I
> > > > didn't find relevant reports.
> > > >
> > > > The steps above are quite artificial. I actually encountered such an
> > > > unexpected denial of access with an in-house application sandbox
> > > > framework; each app has its own dedicated filesystem tree where the
> > > > process's /proc/[pid] is bind-mounted to and the app enters into via
> > > > chroot.
> > > >
> > > > With this patch, writing to /proc/[pid]/attr/current (and its per-security
> > > > module variant) updates the inode security blob of /proc/[pid] or
> > > > /proc/[pid]/task/[tid] (when pid != tid) with the new attribute.
> > > >
> > > > [1] https://lkml.kernel.org/linux-fsdevel/4A2D15AF.8090000@sun.com/
> > > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/selinux/hooks.c#n4220
> > > >
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> > > > ---
> > > >  fs/proc/base.c | 23 ++++++++++++++++++++---
> > > >  1 file changed, 20 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index dd31e3b6bf77..bdb7bea53475 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -2741,6 +2741,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> > > >  {
> > > >     struct inode * inode = file_inode(file);
> > > >     struct task_struct *task;
> > > > +   const char *name = file->f_path.dentry->d_name.name;
> > > >     void *page;
> > > >     int rv;
> > > >
> > > > @@ -2784,10 +2785,26 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> > > >     if (rv < 0)
> > > >             goto out_free;
> > > >
> > > > -   rv = security_setprocattr(PROC_I(inode)->op.lsm,
> > > > -                             file->f_path.dentry->d_name.name, page,
> > > > -                             count);
> > > > +   rv = security_setprocattr(PROC_I(inode)->op.lsm, name, page, count);
> > > >     mutex_unlock(&current->signal->cred_guard_mutex);
> > > > +
> > > > +   /*
> > > > +    *  Update the inode security blob in advance if the task's security
> > > > +    *  attribute was updated
> > > > +    */
> > > > +   if (rv > 0 && !strcmp(name, "current")) {
> > > > +           struct pid *pid;
> > > > +           struct proc_inode *cur, *ei;
> > > > +
> > > > +           rcu_read_lock();
> > > > +           pid = get_task_pid(current, PIDTYPE_PID);
> > > > +           hlist_for_each_entry(cur, &pid->inodes, sibling_inodes)
> > > > +                   ei = cur;
> > >
> > > Should this "break;"? Why is only the last inode in the list updated?
> > > Should it be the first? All of them?
> >
> > If it picks up the first node, it may end up updating /proc/[pid]/task/[tid]
> > rather than /proc/[pid] (when pid == tid) and the task may be denied access
> > to its own /proc/[pid] afterward.
> >
> > I think updating all of them won't hurt. But, as long as /proc/[pid] is
> > accessible, the rest of the inodes should be updated upon path lookup via
> > revalidation as usual.
> >
> > When pid != tid, it only updates /proc/[pid]/task/[tid] and the thread may
> > lose an access to /proc/[pid], but I think it's okay as it's a matter of
> > security policy enforced by security modules. Casey, do you have any
> > comments here?
> >
> > > > +           put_pid(pid);
> > > > +           pid_update_inode(current, &ei->vfs_inode);
> > > > +           rcu_read_unlock();
> > > > +   }
> 
> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> at the top.  However, before we go too much further on this, can we
> get clarification that Casey was able to reproduce this on a stock
> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> this problem on Linux v6.5.
> 
> However, for the moment I'm going to assume this is a real problem, is
> there some reason why the existing pid_revalidate() code is not being
> called in the bind mount case?  From what I can see in the original
> problem report, the path walk seems to work okay when the file is
> accessed directly from /proc, but fails when done on the bind mount.
> Is there some problem with revalidating dentrys on bind mounts?

Hi Paul,

https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/

After reading this thread, I have doubt about solving this in VFS.
Honestly, however, I'm not sure if it's entirely relevant today.

I think this behavior itself is not specific to bind mounts. Though,
/proc/[pid] files are a bit special since its inode security blob has to be
updated whenever the task's security attribute changed and it requries
revalidation.

I also considered .d_weak_revalidate, but it only helps when the final
component of the path is the mountpoint on path lookup; a task will need to
explicitly access the mountpoint once before accessing the files under the
directory... I don't think it's a solution.


Thanks,
Munehisa

> -- 
> paul-moore.com
> 

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Paul Moore]

On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:

...

> > I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> > at the top.  However, before we go too much further on this, can we
> > get clarification that Casey was able to reproduce this on a stock
> > upstream kernel?  Last I read in the other thread Casey wasn't seeing
> > this problem on Linux v6.5.
> >
> > However, for the moment I'm going to assume this is a real problem, is
> > there some reason why the existing pid_revalidate() code is not being
> > called in the bind mount case?  From what I can see in the original
> > problem report, the path walk seems to work okay when the file is
> > accessed directly from /proc, but fails when done on the bind mount.
> > Is there some problem with revalidating dentrys on bind mounts?
>
> Hi Paul,
>
> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
>
> After reading this thread, I have doubt about solving this in VFS.
> Honestly, however, I'm not sure if it's entirely relevant today.

Have you tried simply mounting proc a second time instead of using a bind mount?

 % mount -t proc non /new/location/for/proc

I ask because from your description it appears that proc does the
right thing with respect to revalidation, it only becomes an issue
when accessing proc through a bind mount.  Or did I misunderstand the
problem?

--
paul-moore.com

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Casey Schaufler]

On 12/8/2023 2:43 PM, Paul Moore wrote:
> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> ..
>
>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
>>> at the top.  However, before we go too much further on this, can we
>>> get clarification that Casey was able to reproduce this on a stock
>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
>>> this problem on Linux v6.5.
>>>
>>> However, for the moment I'm going to assume this is a real problem, is
>>> there some reason why the existing pid_revalidate() code is not being
>>> called in the bind mount case?  From what I can see in the original
>>> problem report, the path walk seems to work okay when the file is
>>> accessed directly from /proc, but fails when done on the bind mount.
>>> Is there some problem with revalidating dentrys on bind mounts?
>> Hi Paul,
>>
>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
>>
>> After reading this thread, I have doubt about solving this in VFS.
>> Honestly, however, I'm not sure if it's entirely relevant today.
> Have you tried simply mounting proc a second time instead of using a bind mount?
>
>  % mount -t proc non /new/location/for/proc
>
> I ask because from your description it appears that proc does the
> right thing with respect to revalidation, it only becomes an issue
> when accessing proc through a bind mount.  Or did I misunderstand the
> problem?

It's not hard to make the problem go away by performing some simple
action. I was unable to reproduce the problem initially because I 
checked the Smack label on the bind mounted proc entry before doing
the cat of it. The problem shows up if nothing happens to update the
inode. 

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Paul Moore]

On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 12/8/2023 2:43 PM, Paul Moore wrote:
> > On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> >> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> > ..
> >
> >>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> >>> at the top.  However, before we go too much further on this, can we
> >>> get clarification that Casey was able to reproduce this on a stock
> >>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> >>> this problem on Linux v6.5.
> >>>
> >>> However, for the moment I'm going to assume this is a real problem, is
> >>> there some reason why the existing pid_revalidate() code is not being
> >>> called in the bind mount case?  From what I can see in the original
> >>> problem report, the path walk seems to work okay when the file is
> >>> accessed directly from /proc, but fails when done on the bind mount.
> >>> Is there some problem with revalidating dentrys on bind mounts?
> >> Hi Paul,
> >>
> >> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> >>
> >> After reading this thread, I have doubt about solving this in VFS.
> >> Honestly, however, I'm not sure if it's entirely relevant today.
> > Have you tried simply mounting proc a second time instead of using a bind mount?
> >
> >  % mount -t proc non /new/location/for/proc
> >
> > I ask because from your description it appears that proc does the
> > right thing with respect to revalidation, it only becomes an issue
> > when accessing proc through a bind mount.  Or did I misunderstand the
> > problem?
>
> It's not hard to make the problem go away by performing some simple
> action. I was unable to reproduce the problem initially because I
> checked the Smack label on the bind mounted proc entry before doing
> the cat of it. The problem shows up if nothing happens to update the
> inode.

A good point.

I'm kinda thinking we just leave things as-is, especially since the
proposed fix isn't something anyone is really excited about.

-- 
paul-moore.com

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Casey Schaufler]

On 12/8/2023 3:32 PM, Paul Moore wrote:
> On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 12/8/2023 2:43 PM, Paul Moore wrote:
>>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
>>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
>>> ..
>>>
>>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
>>>>> at the top.  However, before we go too much further on this, can we
>>>>> get clarification that Casey was able to reproduce this on a stock
>>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
>>>>> this problem on Linux v6.5.
>>>>>
>>>>> However, for the moment I'm going to assume this is a real problem, is
>>>>> there some reason why the existing pid_revalidate() code is not being
>>>>> called in the bind mount case?  From what I can see in the original
>>>>> problem report, the path walk seems to work okay when the file is
>>>>> accessed directly from /proc, but fails when done on the bind mount.
>>>>> Is there some problem with revalidating dentrys on bind mounts?
>>>> Hi Paul,
>>>>
>>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
>>>>
>>>> After reading this thread, I have doubt about solving this in VFS.
>>>> Honestly, however, I'm not sure if it's entirely relevant today.
>>> Have you tried simply mounting proc a second time instead of using a bind mount?
>>>
>>>  % mount -t proc non /new/location/for/proc
>>>
>>> I ask because from your description it appears that proc does the
>>> right thing with respect to revalidation, it only becomes an issue
>>> when accessing proc through a bind mount.  Or did I misunderstand the
>>> problem?
>> It's not hard to make the problem go away by performing some simple
>> action. I was unable to reproduce the problem initially because I
>> checked the Smack label on the bind mounted proc entry before doing
>> the cat of it. The problem shows up if nothing happens to update the
>> inode.
> A good point.
>
> I'm kinda thinking we just leave things as-is, especially since the
> proposed fix isn't something anyone is really excited about.

"We have to compromise the performance of our sandboxing tool because of
a kernel bug that's known and for which a fix is available."

If this were just a curiosity that wasn't affecting real development I
might agree. But we've got a real world problem, and I don't see ignoring
it as a good approach. I can't see maintainers of other LSMs thinking so
if this were interfering with their users.

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Munehisa Kamata]

On Sat, 2023-12-09 00:24:42 +0000, Casey Schaufler wrote:
>
> On 12/8/2023 3:32 PM, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> >>> ..
> >>>
> >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> >>>>> at the top.  However, before we go too much further on this, can we
> >>>>> get clarification that Casey was able to reproduce this on a stock
> >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> >>>>> this problem on Linux v6.5.
> >>>>>
> >>>>> However, for the moment I'm going to assume this is a real problem, is
> >>>>> there some reason why the existing pid_revalidate() code is not being
> >>>>> called in the bind mount case?  From what I can see in the original
> >>>>> problem report, the path walk seems to work okay when the file is
> >>>>> accessed directly from /proc, but fails when done on the bind mount.
> >>>>> Is there some problem with revalidating dentrys on bind mounts?
> >>>> Hi Paul,
> >>>>
> >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> >>>>
> >>>> After reading this thread, I have doubt about solving this in VFS.
> >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> >>>
> >>>  % mount -t proc non /new/location/for/proc
> >>>
> >>> I ask because from your description it appears that proc does the
> >>> right thing with respect to revalidation, it only becomes an issue
> >>> when accessing proc through a bind mount.  Or did I misunderstand the
> >>> problem?
> >> It's not hard to make the problem go away by performing some simple
> >> action. I was unable to reproduce the problem initially because I
> >> checked the Smack label on the bind mounted proc entry before doing
> >> the cat of it. The problem shows up if nothing happens to update the
> >> inode.
> > A good point.
> >
> > I'm kinda thinking we just leave things as-is, especially since the
> > proposed fix isn't something anyone is really excited about.
> 
> "We have to compromise the performance of our sandboxing tool because of
> a kernel bug that's known and for which a fix is available."
> 
> If this were just a curiosity that wasn't affecting real development I
> might agree. But we've got a real world problem, and I don't see ignoring
> it as a good approach. I can't see maintainers of other LSMs thinking so
> if this were interfering with their users.
 
We do bind mount to make information exposed to the sandboxed task as little
as possible. We also create a separate PID namespace for each sandbox, but
still want to bind mount even with it to hide system-wide and pid 1
information from the task. 

So, yeah, I see this as a real problem for our use case and want to seek an
opinion about a possibly better fix.


Thanks,
Munehisa 

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Paul Moore]

On Fri, Dec 8, 2023 at 7:24 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 12/8/2023 3:32 PM, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> >>> ..
> >>>
> >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> >>>>> at the top.  However, before we go too much further on this, can we
> >>>>> get clarification that Casey was able to reproduce this on a stock
> >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> >>>>> this problem on Linux v6.5.
> >>>>>
> >>>>> However, for the moment I'm going to assume this is a real problem, is
> >>>>> there some reason why the existing pid_revalidate() code is not being
> >>>>> called in the bind mount case?  From what I can see in the original
> >>>>> problem report, the path walk seems to work okay when the file is
> >>>>> accessed directly from /proc, but fails when done on the bind mount.
> >>>>> Is there some problem with revalidating dentrys on bind mounts?
> >>>> Hi Paul,
> >>>>
> >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> >>>>
> >>>> After reading this thread, I have doubt about solving this in VFS.
> >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> >>>
> >>>  % mount -t proc non /new/location/for/proc
> >>>
> >>> I ask because from your description it appears that proc does the
> >>> right thing with respect to revalidation, it only becomes an issue
> >>> when accessing proc through a bind mount.  Or did I misunderstand the
> >>> problem?
> >> It's not hard to make the problem go away by performing some simple
> >> action. I was unable to reproduce the problem initially because I
> >> checked the Smack label on the bind mounted proc entry before doing
> >> the cat of it. The problem shows up if nothing happens to update the
> >> inode.
> > A good point.
> >
> > I'm kinda thinking we just leave things as-is, especially since the
> > proposed fix isn't something anyone is really excited about.
>
> "We have to compromise the performance of our sandboxing tool because of
> a kernel bug that's known and for which a fix is available."
>
> If this were just a curiosity that wasn't affecting real development I
> might agree. But we've got a real world problem, and I don't see ignoring
> it as a good approach. I can't see maintainers of other LSMs thinking so
> if this were interfering with their users.

While the reproducer may be written for Smack, there are plenty of
indications that this applies to all LSMs and my comments have taken
that into account.

If you're really that upset, try channeling that outrage into your
editor and draft a patch for this that isn't awful.

-- 
paul-moore.com

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Paul Moore]

On Fri, Dec 8, 2023 at 8:11 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> On Sat, 2023-12-09 00:24:42 +0000, Casey Schaufler wrote:
> > On 12/8/2023 3:32 PM, Paul Moore wrote:
> > > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> > >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> > >>> ..
> > >>>
> > >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> > >>>>> at the top.  However, before we go too much further on this, can we
> > >>>>> get clarification that Casey was able to reproduce this on a stock
> > >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> > >>>>> this problem on Linux v6.5.
> > >>>>>
> > >>>>> However, for the moment I'm going to assume this is a real problem, is
> > >>>>> there some reason why the existing pid_revalidate() code is not being
> > >>>>> called in the bind mount case?  From what I can see in the original
> > >>>>> problem report, the path walk seems to work okay when the file is
> > >>>>> accessed directly from /proc, but fails when done on the bind mount.
> > >>>>> Is there some problem with revalidating dentrys on bind mounts?
> > >>>> Hi Paul,
> > >>>>
> > >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> > >>>>
> > >>>> After reading this thread, I have doubt about solving this in VFS.
> > >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> > >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> > >>>
> > >>>  % mount -t proc non /new/location/for/proc
> > >>>
> > >>> I ask because from your description it appears that proc does the
> > >>> right thing with respect to revalidation, it only becomes an issue
> > >>> when accessing proc through a bind mount.  Or did I misunderstand the
> > >>> problem?
> > >> It's not hard to make the problem go away by performing some simple
> > >> action. I was unable to reproduce the problem initially because I
> > >> checked the Smack label on the bind mounted proc entry before doing
> > >> the cat of it. The problem shows up if nothing happens to update the
> > >> inode.
> > > A good point.
> > >
> > > I'm kinda thinking we just leave things as-is, especially since the
> > > proposed fix isn't something anyone is really excited about.
> >
> > "We have to compromise the performance of our sandboxing tool because of
> > a kernel bug that's known and for which a fix is available."
> >
> > If this were just a curiosity that wasn't affecting real development I
> > might agree. But we've got a real world problem, and I don't see ignoring
> > it as a good approach. I can't see maintainers of other LSMs thinking so
> > if this were interfering with their users.
>
> We do bind mount to make information exposed to the sandboxed task as little
> as possible. We also create a separate PID namespace for each sandbox, but
> still want to bind mount even with it to hide system-wide and pid 1
> information from the task.
>
> So, yeah, I see this as a real problem for our use case and want to seek an
> opinion about a possibly better fix.

First, can you confirm that this doesn't happen if you do a second
proc mount instead of a bind mount of the original /proc as requested
previously?

-- 
paul-moore.com

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Casey Schaufler]

On 12/9/2023 10:08 AM, Paul Moore wrote:
> On Fri, Dec 8, 2023 at 7:24 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 12/8/2023 3:32 PM, Paul Moore wrote:
>>> On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 12/8/2023 2:43 PM, Paul Moore wrote:
>>>>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
>>>>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
>>>>> ..
>>>>>
>>>>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
>>>>>>> at the top.  However, before we go too much further on this, can we
>>>>>>> get clarification that Casey was able to reproduce this on a stock
>>>>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
>>>>>>> this problem on Linux v6.5.
>>>>>>>
>>>>>>> However, for the moment I'm going to assume this is a real problem, is
>>>>>>> there some reason why the existing pid_revalidate() code is not being
>>>>>>> called in the bind mount case?  From what I can see in the original
>>>>>>> problem report, the path walk seems to work okay when the file is
>>>>>>> accessed directly from /proc, but fails when done on the bind mount.
>>>>>>> Is there some problem with revalidating dentrys on bind mounts?
>>>>>> Hi Paul,
>>>>>>
>>>>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
>>>>>>
>>>>>> After reading this thread, I have doubt about solving this in VFS.
>>>>>> Honestly, however, I'm not sure if it's entirely relevant today.
>>>>> Have you tried simply mounting proc a second time instead of using a bind mount?
>>>>>
>>>>>  % mount -t proc non /new/location/for/proc
>>>>>
>>>>> I ask because from your description it appears that proc does the
>>>>> right thing with respect to revalidation, it only becomes an issue
>>>>> when accessing proc through a bind mount.  Or did I misunderstand the
>>>>> problem?
>>>> It's not hard to make the problem go away by performing some simple
>>>> action. I was unable to reproduce the problem initially because I
>>>> checked the Smack label on the bind mounted proc entry before doing
>>>> the cat of it. The problem shows up if nothing happens to update the
>>>> inode.
>>> A good point.
>>>
>>> I'm kinda thinking we just leave things as-is, especially since the
>>> proposed fix isn't something anyone is really excited about.
>> "We have to compromise the performance of our sandboxing tool because of
>> a kernel bug that's known and for which a fix is available."
>>
>> If this were just a curiosity that wasn't affecting real development I
>> might agree. But we've got a real world problem, and I don't see ignoring
>> it as a good approach. I can't see maintainers of other LSMs thinking so
>> if this were interfering with their users.
> While the reproducer may be written for Smack, there are plenty of
> indications that this applies to all LSMs and my comments have taken
> that into account.
>
> If you're really that upset, try channeling that outrage into your
> editor and draft a patch for this that isn't awful.

We could "just" wait for the lsm_set_self_attr() syscall to land, and
suggest that it be used instead of the buggy /proc interfaces.

I would love to propose a patch that's less sucky, but have not come
up with one. My understanding of VFS internals isn't up to the task,
I fear.

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Munehisa Kamata]

On Sat, 2023-12-09 10:10:32 -0800, Paul Moore wrote:
>
> On Fri, Dec 8, 2023 at 8:11 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > On Sat, 2023-12-09 00:24:42 +0000, Casey Schaufler wrote:
> > > On 12/8/2023 3:32 PM, Paul Moore wrote:
> > > > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> > > >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > > >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> > > >>> ..
> > > >>>
> > > >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> > > >>>>> at the top.  However, before we go too much further on this, can we
> > > >>>>> get clarification that Casey was able to reproduce this on a stock
> > > >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> > > >>>>> this problem on Linux v6.5.
> > > >>>>>
> > > >>>>> However, for the moment I'm going to assume this is a real problem, is
> > > >>>>> there some reason why the existing pid_revalidate() code is not being
> > > >>>>> called in the bind mount case?  From what I can see in the original
> > > >>>>> problem report, the path walk seems to work okay when the file is
> > > >>>>> accessed directly from /proc, but fails when done on the bind mount.
> > > >>>>> Is there some problem with revalidating dentrys on bind mounts?
> > > >>>> Hi Paul,
> > > >>>>
> > > >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> > > >>>>
> > > >>>> After reading this thread, I have doubt about solving this in VFS.
> > > >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> > > >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> > > >>>
> > > >>>  % mount -t proc non /new/location/for/proc
> > > >>>
> > > >>> I ask because from your description it appears that proc does the
> > > >>> right thing with respect to revalidation, it only becomes an issue
> > > >>> when accessing proc through a bind mount.  Or did I misunderstand the
> > > >>> problem?
> > > >> It's not hard to make the problem go away by performing some simple
> > > >> action. I was unable to reproduce the problem initially because I
> > > >> checked the Smack label on the bind mounted proc entry before doing
> > > >> the cat of it. The problem shows up if nothing happens to update the
> > > >> inode.
> > > > A good point.
> > > >
> > > > I'm kinda thinking we just leave things as-is, especially since the
> > > > proposed fix isn't something anyone is really excited about.
> > >
> > > "We have to compromise the performance of our sandboxing tool because of
> > > a kernel bug that's known and for which a fix is available."
> > >
> > > If this were just a curiosity that wasn't affecting real development I
> > > might agree. But we've got a real world problem, and I don't see ignoring
> > > it as a good approach. I can't see maintainers of other LSMs thinking so
> > > if this were interfering with their users.
> >
> > We do bind mount to make information exposed to the sandboxed task as little
> > as possible. We also create a separate PID namespace for each sandbox, but
> > still want to bind mount even with it to hide system-wide and pid 1
> > information from the task.
> >
> > So, yeah, I see this as a real problem for our use case and want to seek an
> > opinion about a possibly better fix.
> 
> First, can you confirm that this doesn't happen if you do a second
> proc mount instead of a bind mount of the original /proc as requested
> previously?

Mounting the entire /proc was considered and this doesn't happen with it.
Although we still prefer to do bind mount for the reasons above and then
seek a solution.


Thanks,
Munehisa

> -- 
> paul-moore.com
> 

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Munehisa Kamata]

On Sat, 2023-12-09 10:35:01 -0800, Casey Schaufler wrote:
>
> On 12/9/2023 10:08 AM, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 7:24 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 12/8/2023 3:32 PM, Paul Moore wrote:
> >>> On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> On 12/8/2023 2:43 PM, Paul Moore wrote:
> >>>>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> >>>>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> >>>>> ..
> >>>>>
> >>>>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> >>>>>>> at the top.  However, before we go too much further on this, can we
> >>>>>>> get clarification that Casey was able to reproduce this on a stock
> >>>>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> >>>>>>> this problem on Linux v6.5.
> >>>>>>>
> >>>>>>> However, for the moment I'm going to assume this is a real problem, is
> >>>>>>> there some reason why the existing pid_revalidate() code is not being
> >>>>>>> called in the bind mount case?  From what I can see in the original
> >>>>>>> problem report, the path walk seems to work okay when the file is
> >>>>>>> accessed directly from /proc, but fails when done on the bind mount.
> >>>>>>> Is there some problem with revalidating dentrys on bind mounts?
> >>>>>> Hi Paul,
> >>>>>>
> >>>>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> >>>>>>
> >>>>>> After reading this thread, I have doubt about solving this in VFS.
> >>>>>> Honestly, however, I'm not sure if it's entirely relevant today.
> >>>>> Have you tried simply mounting proc a second time instead of using a bind mount?
> >>>>>
> >>>>>  % mount -t proc non /new/location/for/proc
> >>>>>
> >>>>> I ask because from your description it appears that proc does the
> >>>>> right thing with respect to revalidation, it only becomes an issue
> >>>>> when accessing proc through a bind mount.  Or did I misunderstand the
> >>>>> problem?
> >>>> It's not hard to make the problem go away by performing some simple
> >>>> action. I was unable to reproduce the problem initially because I
> >>>> checked the Smack label on the bind mounted proc entry before doing
> >>>> the cat of it. The problem shows up if nothing happens to update the
> >>>> inode.
> >>> A good point.
> >>>
> >>> I'm kinda thinking we just leave things as-is, especially since the
> >>> proposed fix isn't something anyone is really excited about.
> >> "We have to compromise the performance of our sandboxing tool because of
> >> a kernel bug that's known and for which a fix is available."
> >>
> >> If this were just a curiosity that wasn't affecting real development I
> >> might agree. But we've got a real world problem, and I don't see ignoring
> >> it as a good approach. I can't see maintainers of other LSMs thinking so
> >> if this were interfering with their users.
> > While the reproducer may be written for Smack, there are plenty of
> > indications that this applies to all LSMs and my comments have taken
> > that into account.
> >
> > If you're really that upset, try channeling that outrage into your
> > editor and draft a patch for this that isn't awful.
> 
> We could "just" wait for the lsm_set_self_attr() syscall to land, and
> suggest that it be used instead of the buggy /proc interfaces.
> 
> I would love to propose a patch that's less sucky, but have not come
> up with one. My understanding of VFS internals isn't up to the task,
> I fear.

As an another option, perhaps adding an even stricter hidepid mode in
procfs (and avoid bind mount) could be reasonable?

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Serge E. Hallyn]

On Sat, Dec 09, 2023 at 01:10:42AM +0000, Munehisa Kamata wrote:
> On Sat, 2023-12-09 00:24:42 +0000, Casey Schaufler wrote:
> >
> > On 12/8/2023 3:32 PM, Paul Moore wrote:
> > > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> > >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> > >>> ..
> > >>>
> > >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> > >>>>> at the top.  However, before we go too much further on this, can we
> > >>>>> get clarification that Casey was able to reproduce this on a stock
> > >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> > >>>>> this problem on Linux v6.5.
> > >>>>>
> > >>>>> However, for the moment I'm going to assume this is a real problem, is
> > >>>>> there some reason why the existing pid_revalidate() code is not being
> > >>>>> called in the bind mount case?  From what I can see in the original
> > >>>>> problem report, the path walk seems to work okay when the file is
> > >>>>> accessed directly from /proc, but fails when done on the bind mount.
> > >>>>> Is there some problem with revalidating dentrys on bind mounts?
> > >>>> Hi Paul,
> > >>>>
> > >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> > >>>>
> > >>>> After reading this thread, I have doubt about solving this in VFS.
> > >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> > >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> > >>>
> > >>>  % mount -t proc non /new/location/for/proc
> > >>>
> > >>> I ask because from your description it appears that proc does the
> > >>> right thing with respect to revalidation, it only becomes an issue
> > >>> when accessing proc through a bind mount.  Or did I misunderstand the
> > >>> problem?
> > >> It's not hard to make the problem go away by performing some simple
> > >> action. I was unable to reproduce the problem initially because I
> > >> checked the Smack label on the bind mounted proc entry before doing
> > >> the cat of it. The problem shows up if nothing happens to update the
> > >> inode.
> > > A good point.
> > >
> > > I'm kinda thinking we just leave things as-is, especially since the
> > > proposed fix isn't something anyone is really excited about.
> > 
> > "We have to compromise the performance of our sandboxing tool because of
> > a kernel bug that's known and for which a fix is available."
> > 
> > If this were just a curiosity that wasn't affecting real development I
> > might agree. But we've got a real world problem, and I don't see ignoring
> > it as a good approach. I can't see maintainers of other LSMs thinking so
> > if this were interfering with their users.
>  
> We do bind mount to make information exposed to the sandboxed task as little
> as possible. We also create a separate PID namespace for each sandbox, but

If not exposing information is the main motivation, then could you simply do:

mount -t proc proc dir
mount --bind dir/$$ dir

?

> still want to bind mount even with it to hide system-wide and pid 1
> information from the task. 
> 
> So, yeah, I see this as a real problem for our use case and want to seek an
> opinion about a possibly better fix.
> 
> 
> Thanks,
> Munehisa 

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Paul Moore]

On Sat, Dec 9, 2023 at 1:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 12/9/2023 10:08 AM, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 7:24 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 12/8/2023 3:32 PM, Paul Moore wrote:
> >>> On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> On 12/8/2023 2:43 PM, Paul Moore wrote:
> >>>>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> >>>>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> >>>>> ..
> >>>>>
> >>>>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> >>>>>>> at the top.  However, before we go too much further on this, can we
> >>>>>>> get clarification that Casey was able to reproduce this on a stock
> >>>>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> >>>>>>> this problem on Linux v6.5.
> >>>>>>>
> >>>>>>> However, for the moment I'm going to assume this is a real problem, is
> >>>>>>> there some reason why the existing pid_revalidate() code is not being
> >>>>>>> called in the bind mount case?  From what I can see in the original
> >>>>>>> problem report, the path walk seems to work okay when the file is
> >>>>>>> accessed directly from /proc, but fails when done on the bind mount.
> >>>>>>> Is there some problem with revalidating dentrys on bind mounts?
> >>>>>> Hi Paul,
> >>>>>>
> >>>>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> >>>>>>
> >>>>>> After reading this thread, I have doubt about solving this in VFS.
> >>>>>> Honestly, however, I'm not sure if it's entirely relevant today.
> >>>>> Have you tried simply mounting proc a second time instead of using a bind mount?
> >>>>>
> >>>>>  % mount -t proc non /new/location/for/proc
> >>>>>
> >>>>> I ask because from your description it appears that proc does the
> >>>>> right thing with respect to revalidation, it only becomes an issue
> >>>>> when accessing proc through a bind mount.  Or did I misunderstand the
> >>>>> problem?
> >>>> It's not hard to make the problem go away by performing some simple
> >>>> action. I was unable to reproduce the problem initially because I
> >>>> checked the Smack label on the bind mounted proc entry before doing
> >>>> the cat of it. The problem shows up if nothing happens to update the
> >>>> inode.
> >>> A good point.
> >>>
> >>> I'm kinda thinking we just leave things as-is, especially since the
> >>> proposed fix isn't something anyone is really excited about.
> >> "We have to compromise the performance of our sandboxing tool because of
> >> a kernel bug that's known and for which a fix is available."
> >>
> >> If this were just a curiosity that wasn't affecting real development I
> >> might agree. But we've got a real world problem, and I don't see ignoring
> >> it as a good approach. I can't see maintainers of other LSMs thinking so
> >> if this were interfering with their users.
> > While the reproducer may be written for Smack, there are plenty of
> > indications that this applies to all LSMs and my comments have taken
> > that into account.
> >
> > If you're really that upset, try channeling that outrage into your
> > editor and draft a patch for this that isn't awful.
>
> We could "just" wait for the lsm_set_self_attr() syscall to land, and
> suggest that it be used instead of the buggy /proc interfaces.

You didn't like the "as-is" approach before, I'm a little surprised
with the change in heart in less than 24 hours ...

> I would love to propose a patch that's less sucky, but have not come
> up with one. My understanding of VFS internals isn't up to the task,
> I fear.

I'm not convinced we've properly identified the root cause of the
problem, once we have it might be easier to identify a solution.

-- 
paul-moore.com

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Paul Moore]

On Sat, Dec 9, 2023 at 4:17 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> On Sat, 2023-12-09 10:10:32 -0800, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 8:11 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > > On Sat, 2023-12-09 00:24:42 +0000, Casey Schaufler wrote:
> > > > On 12/8/2023 3:32 PM, Paul Moore wrote:
> > > > > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> > > > >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > > > >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> > > > >>> ..
> > > > >>>
> > > > >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> > > > >>>>> at the top.  However, before we go too much further on this, can we
> > > > >>>>> get clarification that Casey was able to reproduce this on a stock
> > > > >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> > > > >>>>> this problem on Linux v6.5.
> > > > >>>>>
> > > > >>>>> However, for the moment I'm going to assume this is a real problem, is
> > > > >>>>> there some reason why the existing pid_revalidate() code is not being
> > > > >>>>> called in the bind mount case?  From what I can see in the original
> > > > >>>>> problem report, the path walk seems to work okay when the file is
> > > > >>>>> accessed directly from /proc, but fails when done on the bind mount.
> > > > >>>>> Is there some problem with revalidating dentrys on bind mounts?
> > > > >>>> Hi Paul,
> > > > >>>>
> > > > >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> > > > >>>>
> > > > >>>> After reading this thread, I have doubt about solving this in VFS.
> > > > >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> > > > >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> > > > >>>
> > > > >>>  % mount -t proc non /new/location/for/proc
> > > > >>>
> > > > >>> I ask because from your description it appears that proc does the
> > > > >>> right thing with respect to revalidation, it only becomes an issue
> > > > >>> when accessing proc through a bind mount.  Or did I misunderstand the
> > > > >>> problem?
> > > > >> It's not hard to make the problem go away by performing some simple
> > > > >> action. I was unable to reproduce the problem initially because I
> > > > >> checked the Smack label on the bind mounted proc entry before doing
> > > > >> the cat of it. The problem shows up if nothing happens to update the
> > > > >> inode.
> > > > > A good point.
> > > > >
> > > > > I'm kinda thinking we just leave things as-is, especially since the
> > > > > proposed fix isn't something anyone is really excited about.
> > > >
> > > > "We have to compromise the performance of our sandboxing tool because of
> > > > a kernel bug that's known and for which a fix is available."
> > > >
> > > > If this were just a curiosity that wasn't affecting real development I
> > > > might agree. But we've got a real world problem, and I don't see ignoring
> > > > it as a good approach. I can't see maintainers of other LSMs thinking so
> > > > if this were interfering with their users.
> > >
> > > We do bind mount to make information exposed to the sandboxed task as little
> > > as possible. We also create a separate PID namespace for each sandbox, but
> > > still want to bind mount even with it to hide system-wide and pid 1
> > > information from the task.
> > >
> > > So, yeah, I see this as a real problem for our use case and want to seek an
> > > opinion about a possibly better fix.
> >
> > First, can you confirm that this doesn't happen if you do a second
> > proc mount instead of a bind mount of the original /proc as requested
> > previously?
>
> Mounting the entire /proc was considered and this doesn't happen with it.
> Although we still prefer to do bind mount for the reasons above and then
> seek a solution.

Ah, I had forgotten that you aren't bind mounting all of /proc, only a
PID specific directory.  I guess I'm not surprised this is behaving a
little odd in some corner cases and I'm even less inclined to support
a hack patch to handle this case; if we're going to fully support
this, the patch will need to be pretty clean.

-- 
paul-moore.com

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Munehisa Kamata]

On Sun, 2023-12-10 06:45:30 -0800, "Serge E. Hallyn" wrote:
>
> On Sat, Dec 09, 2023 at 01:10:42AM +0000, Munehisa Kamata wrote:
> > On Sat, 2023-12-09 00:24:42 +0000, Casey Schaufler wrote:
> > >
> > > On 12/8/2023 3:32 PM, Paul Moore wrote:
> > > > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> > > >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > > >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> > > >>> ..
> > > >>>
> > > >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> > > >>>>> at the top.  However, before we go too much further on this, can we
> > > >>>>> get clarification that Casey was able to reproduce this on a stock
> > > >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> > > >>>>> this problem on Linux v6.5.
> > > >>>>>
> > > >>>>> However, for the moment I'm going to assume this is a real problem, is
> > > >>>>> there some reason why the existing pid_revalidate() code is not being
> > > >>>>> called in the bind mount case?  From what I can see in the original
> > > >>>>> problem report, the path walk seems to work okay when the file is
> > > >>>>> accessed directly from /proc, but fails when done on the bind mount.
> > > >>>>> Is there some problem with revalidating dentrys on bind mounts?
> > > >>>> Hi Paul,
> > > >>>>
> > > >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> > > >>>>
> > > >>>> After reading this thread, I have doubt about solving this in VFS.
> > > >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> > > >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> > > >>>
> > > >>>  % mount -t proc non /new/location/for/proc
> > > >>>
> > > >>> I ask because from your description it appears that proc does the
> > > >>> right thing with respect to revalidation, it only becomes an issue
> > > >>> when accessing proc through a bind mount.  Or did I misunderstand the
> > > >>> problem?
> > > >> It's not hard to make the problem go away by performing some simple
> > > >> action. I was unable to reproduce the problem initially because I
> > > >> checked the Smack label on the bind mounted proc entry before doing
> > > >> the cat of it. The problem shows up if nothing happens to update the
> > > >> inode.
> > > > A good point.
> > > >
> > > > I'm kinda thinking we just leave things as-is, especially since the
> > > > proposed fix isn't something anyone is really excited about.
> > > 
> > > "We have to compromise the performance of our sandboxing tool because of
> > > a kernel bug that's known and for which a fix is available."
> > > 
> > > If this were just a curiosity that wasn't affecting real development I
> > > might agree. But we've got a real world problem, and I don't see ignoring
> > > it as a good approach. I can't see maintainers of other LSMs thinking so
> > > if this were interfering with their users.
> >  
> > We do bind mount to make information exposed to the sandboxed task as little
> > as possible. We also create a separate PID namespace for each sandbox, but
> 
> If not exposing information is the main motivation, then could you simply do:
> 
> mount -t proc proc dir
> mount --bind dir/$$ dir
> 
> ?

Hi Serge,

It doesn't work.

 [root@ip-10-0-32-198 ec2-user]# mount -t proc proc dir
 [root@ip-10-0-32-198 ec2-user]# echo AAA > dir/$$/attr/current
 [root@ip-10-0-32-198 ec2-user]# chsmack dir/$$
 dir/11222 access="AAA"
 [root@ip-10-0-32-198 ec2-user]# mount --bind dir/$$ dir
 [root@ip-10-0-32-198 ec2-user]# echo BBB > dir/attr/current
 [root@ip-10-0-32-198 ec2-user]# echo CCC > dir/attr/current
 bash: dir/attr/current: Permission denied
 [root@ip-10-0-32-198 ec2-user]# ls dir
 ls: cannot access dir: Permission denied
 [root@ip-10-0-32-198 ec2-user]# 
 
It would not revalidate dir/$$ anyway, so this result wasn't surprising to
me. Maybe I'm missing something?


> > still want to bind mount even with it to hide system-wide and pid 1
> > information from the task. 
> > 
> > So, yeah, I see this as a real problem for our use case and want to seek an
> > opinion about a possibly better fix.
> > 
> > 
> > Thanks,
> > Munehisa 
> 

Re: Fw: [PATCH] proc: Update inode upon changing task security attribute

[Serge E. Hallyn]

On Mon, Dec 11, 2023 at 07:27:23PM +0000, Munehisa Kamata wrote:
> On Sun, 2023-12-10 06:45:30 -0800, "Serge E. Hallyn" wrote:
> >
> > On Sat, Dec 09, 2023 at 01:10:42AM +0000, Munehisa Kamata wrote:
> > > On Sat, 2023-12-09 00:24:42 +0000, Casey Schaufler wrote:
> > > >
> > > > On 12/8/2023 3:32 PM, Paul Moore wrote:
> > > > > On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >> On 12/8/2023 2:43 PM, Paul Moore wrote:
> > > > >>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@amazon.com> wrote:
> > > > >>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote:
> > > > >>> ..
> > > > >>>
> > > > >>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> > > > >>>>> at the top.  However, before we go too much further on this, can we
> > > > >>>>> get clarification that Casey was able to reproduce this on a stock
> > > > >>>>> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> > > > >>>>> this problem on Linux v6.5.
> > > > >>>>>
> > > > >>>>> However, for the moment I'm going to assume this is a real problem, is
> > > > >>>>> there some reason why the existing pid_revalidate() code is not being
> > > > >>>>> called in the bind mount case?  From what I can see in the original
> > > > >>>>> problem report, the path walk seems to work okay when the file is
> > > > >>>>> accessed directly from /proc, but fails when done on the bind mount.
> > > > >>>>> Is there some problem with revalidating dentrys on bind mounts?
> > > > >>>> Hi Paul,
> > > > >>>>
> > > > >>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@ZenIV.linux.org.uk/
> > > > >>>>
> > > > >>>> After reading this thread, I have doubt about solving this in VFS.
> > > > >>>> Honestly, however, I'm not sure if it's entirely relevant today.
> > > > >>> Have you tried simply mounting proc a second time instead of using a bind mount?
> > > > >>>
> > > > >>>  % mount -t proc non /new/location/for/proc
> > > > >>>
> > > > >>> I ask because from your description it appears that proc does the
> > > > >>> right thing with respect to revalidation, it only becomes an issue
> > > > >>> when accessing proc through a bind mount.  Or did I misunderstand the
> > > > >>> problem?
> > > > >> It's not hard to make the problem go away by performing some simple
> > > > >> action. I was unable to reproduce the problem initially because I
> > > > >> checked the Smack label on the bind mounted proc entry before doing
> > > > >> the cat of it. The problem shows up if nothing happens to update the
> > > > >> inode.
> > > > > A good point.
> > > > >
> > > > > I'm kinda thinking we just leave things as-is, especially since the
> > > > > proposed fix isn't something anyone is really excited about.
> > > > 
> > > > "We have to compromise the performance of our sandboxing tool because of
> > > > a kernel bug that's known and for which a fix is available."
> > > > 
> > > > If this were just a curiosity that wasn't affecting real development I
> > > > might agree. But we've got a real world problem, and I don't see ignoring
> > > > it as a good approach. I can't see maintainers of other LSMs thinking so
> > > > if this were interfering with their users.
> > >  
> > > We do bind mount to make information exposed to the sandboxed task as little
> > > as possible. We also create a separate PID namespace for each sandbox, but
> > 
> > If not exposing information is the main motivation, then could you simply do:
> > 
> > mount -t proc proc dir
> > mount --bind dir/$$ dir
> > 
> > ?
> 
> Hi Serge,
> 
> It doesn't work.
> 
>  [root@ip-10-0-32-198 ec2-user]# mount -t proc proc dir
>  [root@ip-10-0-32-198 ec2-user]# echo AAA > dir/$$/attr/current
>  [root@ip-10-0-32-198 ec2-user]# chsmack dir/$$
>  dir/11222 access="AAA"
>  [root@ip-10-0-32-198 ec2-user]# mount --bind dir/$$ dir
>  [root@ip-10-0-32-198 ec2-user]# echo BBB > dir/attr/current
>  [root@ip-10-0-32-198 ec2-user]# echo CCC > dir/attr/current
>  bash: dir/attr/current: Permission denied
>  [root@ip-10-0-32-198 ec2-user]# ls dir
>  ls: cannot access dir: Permission denied
>  [root@ip-10-0-32-198 ec2-user]# 
>  
> It would not revalidate dir/$$ anyway, so this result wasn't surprising to
> me. Maybe I'm missing something?

I see.  Yeah, that's an ugly wart.

> > > still want to bind mount even with it to hide system-wide and pid 1
> > > information from the task. 
> > > 
> > > So, yeah, I see this as a real problem for our use case and want to seek an
> > > opinion about a possibly better fix.
> > > 
> > > 
> > > Thanks,
> > > Munehisa 
> >