[v2] tomoyo: Don't check open/getattr permission on sockets.

Commit Message

Tetsuo Handa June 22, 2019, 4:45 a.m. UTC

On 2019/06/19 5:49, Al Viro wrote:
> On Sun, Jun 16, 2019 at 03:49:00PM +0900, Tetsuo Handa wrote:
>> Hello, Al.
>>
>> Q1: Do you agree that we should fix TOMOYO side rather than SOCKET_I()->sk
>>     management.
> 
> You do realize that sockets are not unique in that respect, right?
> All kinds of interesting stuff can be accessed via /proc/*/fd/*, and
> it _can_ be closed under you.  So I'd suggest checking how your code
> copes with similar for pipes, FIFOs, epoll, etc., accessed that way...

I know all kinds of interesting stuff can be accessed via /proc/*/fd/*,
and it _can_ be closed under me.

Regarding sockets, I was accessing "struct socket" memory and
"struct sock" memory which are outside of "struct inode" memory.

But regarding other objects, I am accessing "struct dentry" memory,
"struct super_block" memory and "struct inode" memory. I'm expecting
that these memory can't be kfree()d as long as "struct path" holds
a reference. Is my expectation correct?

> 
> We are _not_ going to be checking that in fs/open.c - the stuff found
> via /proc/*/fd/* can have the associated file closed by the time
> we get to calling ->open() and we won't know that until said call.

OK. Then, fixing TOMOYO side is the correct way.

> 
>> Q2: Do you see any problem with using f->f_path.dentry->d_inode ?
>>     Do we need to use d_backing_inode() or d_inode() ?
> 
> Huh?  What's wrong with file_inode(f), in the first place?  And
> just when can that be NULL, while we are at it?

Oh, I was not aware of file_inode(). Thanks.

> 
>>>  static int tomoyo_inode_getattr(const struct path *path)
>>>  {
>>> +	/* It is not safe to call tomoyo_get_socket_name(). */
>>> +	if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
>>> +		return 0;
> 
> Can that be called for a negative?
> 

I check for NULL when I'm not sure it is guaranteed to hold a valid pointer.
You meant "we are sure that path->dentry->d_inode is valid", don't you?

By the way, "negative" associates with IS_ERR() range. I guess that
"NULL" is the better name...

Anyway, here is V2 patch.

From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[e-mail]>
Date: Sat, 22 Jun 2019 13:14:26 +0900
Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.

syzbot is reporting that use of SOCKET_I()->sk from open() can result in
use after free problem [1], for socket's inode is still reachable via
/proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.

But there is no point with calling security_file_open() on sockets
because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.

There is some point with calling security_inode_getattr() on sockets
because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
are valid. If we want to access "struct sock"->sk_{family,type,protocol}
fields, we will need to use security_socket_post_create() hook and
security_inode_free() hook in order to remember these fields because
security_sk_free() hook is called before the inode is destructed. But
since information which can be protected by checking
security_inode_getattr() on sockets is trivial, let's not be bothered by
"struct inode"->i_security management.

There is point with calling security_file_ioctl() on sockets. Since
ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
on sockets should remain safe.

[1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74

Signed-off-by: Tetsuo Handa <[e-mail]>
Reported-by: syzbot <[e-mail]>
---
 security/tomoyo/tomoyo.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Tetsuo Handa July 4, 2019, 11:58 a.m. UTC | #1

Hello.

Since it seems that Al has no comments, I'd like to send this patch to
linux-next.git . What should I do? Do I need to set up a git tree?

On 2019/06/22 13:45, Tetsuo Handa wrote:
> From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <[e-mail]>
> Date: Sat, 22 Jun 2019 13:14:26 +0900
> Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
> 
> syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> use after free problem [1], for socket's inode is still reachable via
> /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
> 
> But there is no point with calling security_file_open() on sockets
> because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
> 
> There is some point with calling security_inode_getattr() on sockets
> because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> fields, we will need to use security_socket_post_create() hook and
> security_inode_free() hook in order to remember these fields because
> security_sk_free() hook is called before the inode is destructed. But
> since information which can be protected by checking
> security_inode_getattr() on sockets is trivial, let's not be bothered by
> "struct inode"->i_security management.
> 
> There is point with calling security_file_ioctl() on sockets. Since
> ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> on sockets should remain safe.
> 
> [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> 
> Signed-off-by: Tetsuo Handa <[e-mail]>
> Reported-by: syzbot <[e-mail]>
> ---
>  security/tomoyo/tomoyo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 716c92e..8ea3f5d 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
>   */
>  static int tomoyo_inode_getattr(const struct path *path)
>  {
> +	/* It is not safe to call tomoyo_get_socket_name(). */
> +	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
> +		return 0;
>  	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
>  }
>  
> @@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
>  	/* Don't check read permission here if called from do_execve(). */
>  	if (current->in_execve)
>  		return 0;
> +	/* Sockets can't be opened by open(). */
> +	if (S_ISSOCK(file_inode(f)->i_mode))
> +		return 0;
>  	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
>  					    f->f_flags);
>  }
>

James Morris July 7, 2019, 2:44 a.m. UTC | #2

On Thu, 4 Jul 2019, Tetsuo Handa wrote:

> Hello.
> 
> Since it seems that Al has no comments, I'd like to send this patch to
> linux-next.git . What should I do? Do I need to set up a git tree?

Yes, you can create one at github or similar.


> 
> On 2019/06/22 13:45, Tetsuo Handa wrote:
> > From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <[e-mail]>
> > Date: Sat, 22 Jun 2019 13:14:26 +0900
> > Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
> > 
> > syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> > use after free problem [1], for socket's inode is still reachable via
> > /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
> > 
> > But there is no point with calling security_file_open() on sockets
> > because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
> > 
> > There is some point with calling security_inode_getattr() on sockets
> > because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> > are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> > fields, we will need to use security_socket_post_create() hook and
> > security_inode_free() hook in order to remember these fields because
> > security_sk_free() hook is called before the inode is destructed. But
> > since information which can be protected by checking
> > security_inode_getattr() on sockets is trivial, let's not be bothered by
> > "struct inode"->i_security management.
> > 
> > There is point with calling security_file_ioctl() on sockets. Since
> > ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> > on sockets should remain safe.
> > 
> > [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> > 
> > Signed-off-by: Tetsuo Handa <[e-mail]>
> > Reported-by: syzbot <[e-mail]>
> > ---
> >  security/tomoyo/tomoyo.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> > index 716c92e..8ea3f5d 100644
> > --- a/security/tomoyo/tomoyo.c
> > +++ b/security/tomoyo/tomoyo.c
> > @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
> >   */
> >  static int tomoyo_inode_getattr(const struct path *path)
> >  {
> > +	/* It is not safe to call tomoyo_get_socket_name(). */
> > +	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
> > +		return 0;
> >  	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
> >  }
> >  
> > @@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
> >  	/* Don't check read permission here if called from do_execve(). */
> >  	if (current->in_execve)
> >  		return 0;
> > +	/* Sockets can't be opened by open(). */
> > +	if (S_ISSOCK(file_inode(f)->i_mode))
> > +		return 0;
> >  	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
> >  					    f->f_flags);
> >  }
> > 
>

James Morris July 7, 2019, 2:50 a.m. UTC | #3

On Sat, 6 Jul 2019, James Morris wrote:

> On Thu, 4 Jul 2019, Tetsuo Handa wrote:
> 
> > Hello.
> > 
> > Since it seems that Al has no comments, I'd like to send this patch to
> > linux-next.git . What should I do? Do I need to set up a git tree?
> 
> Yes, you can create one at github or similar.

Also notify Stephen Rothwell of the location of your -next branch, so it 
gets pulled into his tree.

Eric Biggers Aug. 22, 2019, 6:30 a.m. UTC | #4

Hi Tetsuo,

On Sat, Jun 22, 2019 at 01:45:30PM +0900, Tetsuo Handa wrote:
> On 2019/06/19 5:49, Al Viro wrote:
> > On Sun, Jun 16, 2019 at 03:49:00PM +0900, Tetsuo Handa wrote:
> >> Hello, Al.
> >>
> >> Q1: Do you agree that we should fix TOMOYO side rather than SOCKET_I()->sk
> >>     management.
> > 
> > You do realize that sockets are not unique in that respect, right?
> > All kinds of interesting stuff can be accessed via /proc/*/fd/*, and
> > it _can_ be closed under you.  So I'd suggest checking how your code
> > copes with similar for pipes, FIFOs, epoll, etc., accessed that way...
> 
> I know all kinds of interesting stuff can be accessed via /proc/*/fd/*,
> and it _can_ be closed under me.
> 
> Regarding sockets, I was accessing "struct socket" memory and
> "struct sock" memory which are outside of "struct inode" memory.
> 
> But regarding other objects, I am accessing "struct dentry" memory,
> "struct super_block" memory and "struct inode" memory. I'm expecting
> that these memory can't be kfree()d as long as "struct path" holds
> a reference. Is my expectation correct?
> 
> > 
> > We are _not_ going to be checking that in fs/open.c - the stuff found
> > via /proc/*/fd/* can have the associated file closed by the time
> > we get to calling ->open() and we won't know that until said call.
> 
> OK. Then, fixing TOMOYO side is the correct way.
> 
> > 
> >> Q2: Do you see any problem with using f->f_path.dentry->d_inode ?
> >>     Do we need to use d_backing_inode() or d_inode() ?
> > 
> > Huh?  What's wrong with file_inode(f), in the first place?  And
> > just when can that be NULL, while we are at it?
> 
> Oh, I was not aware of file_inode(). Thanks.
> 
> > 
> >>>  static int tomoyo_inode_getattr(const struct path *path)
> >>>  {
> >>> +	/* It is not safe to call tomoyo_get_socket_name(). */
> >>> +	if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
> >>> +		return 0;
> > 
> > Can that be called for a negative?
> > 
> 
> I check for NULL when I'm not sure it is guaranteed to hold a valid pointer.
> You meant "we are sure that path->dentry->d_inode is valid", don't you?
> 
> By the way, "negative" associates with IS_ERR() range. I guess that
> "NULL" is the better name...
> 
> Anyway, here is V2 patch.
> 
> From c63c4074300921d6d1c33c3b8dc9c84ebfededf5 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <[e-mail]>
> Date: Sat, 22 Jun 2019 13:14:26 +0900
> Subject: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
> 
> syzbot is reporting that use of SOCKET_I()->sk from open() can result in
> use after free problem [1], for socket's inode is still reachable via
> /proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
> 
> But there is no point with calling security_file_open() on sockets
> because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
> 
> There is some point with calling security_inode_getattr() on sockets
> because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
> are valid. If we want to access "struct sock"->sk_{family,type,protocol}
> fields, we will need to use security_socket_post_create() hook and
> security_inode_free() hook in order to remember these fields because
> security_sk_free() hook is called before the inode is destructed. But
> since information which can be protected by checking
> security_inode_getattr() on sockets is trivial, let's not be bothered by
> "struct inode"->i_security management.
> 
> There is point with calling security_file_ioctl() on sockets. Since
> ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
> on sockets should remain safe.
> 
> [1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> 
> Signed-off-by: Tetsuo Handa <[e-mail]>
> Reported-by: syzbot <[e-mail]>
> ---
>  security/tomoyo/tomoyo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 716c92e..8ea3f5d 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
>   */
>  static int tomoyo_inode_getattr(const struct path *path)
>  {
> +	/* It is not safe to call tomoyo_get_socket_name(). */
> +	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
> +		return 0;
>  	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
>  }
>  
> @@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
>  	/* Don't check read permission here if called from do_execve(). */
>  	if (current->in_execve)
>  		return 0;
> +	/* Sockets can't be opened by open(). */
> +	if (S_ISSOCK(file_inode(f)->i_mode))
> +		return 0;
>  	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
>  					    f->f_flags);
>  }
> -- 

What happened to this patch?

Also, isn't the same bug in other places too?:

	- tomoyo_path_chmod()
	- tomoyo_path_chown()
	- smack_inode_getsecurity()
	- smack_inode_setsecurity()

- Eric

Tetsuo Handa Aug. 22, 2019, 6:55 a.m. UTC | #5

Eric Biggers wrote:
> What happened to this patch?

I have to learn how to manage a git tree for sending
pull requests, but I can't find time to try.

> 
> Also, isn't the same bug in other places too?:
> 
> 	- tomoyo_path_chmod()
> 	- tomoyo_path_chown()
> 	- smack_inode_getsecurity()
> 	- smack_inode_setsecurity()

What's the bug? The file descriptor returned by open(O_PATH) cannot be
passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.

Eric Biggers Aug. 22, 2019, 7:01 a.m. UTC | #6

On Thu, Aug 22, 2019 at 03:55:31PM +0900, Tetsuo Handa wrote:
> Eric Biggers wrote:
> > What happened to this patch?
> 
> I have to learn how to manage a git tree for sending
> pull requests, but I can't find time to try.
> 
> > 
> > Also, isn't the same bug in other places too?:
> > 
> > 	- tomoyo_path_chmod()
> > 	- tomoyo_path_chown()
> > 	- smack_inode_getsecurity()
> > 	- smack_inode_setsecurity()
> 
> What's the bug? The file descriptor returned by open(O_PATH) cannot be
> passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.
> 

chmod(2), chown(2), getxattr(2), and setxattr(2) take a path, not a fd.

- Eric

Tetsuo Handa Aug. 22, 2019, 7:42 a.m. UTC | #7

Eric Biggers wrote:
> On Thu, Aug 22, 2019 at 03:55:31PM +0900, Tetsuo Handa wrote:
> > > Also, isn't the same bug in other places too?:
> > > 
> > > 	- tomoyo_path_chmod()
> > > 	- tomoyo_path_chown()
> > > 	- smack_inode_getsecurity()
> > > 	- smack_inode_setsecurity()
> > 
> > What's the bug? The file descriptor returned by open(O_PATH) cannot be
> > passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.
> > 
> 
> chmod(2), chown(2), getxattr(2), and setxattr(2) take a path, not a fd.
> 

OK. Then, is the correct fix

  inode_lock(inode);
  if (SOCKET_I(inode)->sk) {
    // Can access SOCKET_I(sock)->sk->*
  } else {
    // Already close()d. Don't touch.
  }
  inode_unlock(inode);

thanks to

  commit 6d8c50dcb029872b ("socket: close race condition between sock_close() and sockfs_setattr()")
  commit ff7b11aa481f682e ("net: socket: set sock->sk to NULL after calling proto_ops::release()")

changes?

Eric Biggers Aug. 22, 2019, 3:47 p.m. UTC | #8

On Thu, Aug 22, 2019 at 04:42:26PM +0900, Tetsuo Handa wrote:
> Eric Biggers wrote:
> > On Thu, Aug 22, 2019 at 03:55:31PM +0900, Tetsuo Handa wrote:
> > > > Also, isn't the same bug in other places too?:
> > > > 
> > > > 	- tomoyo_path_chmod()
> > > > 	- tomoyo_path_chown()
> > > > 	- smack_inode_getsecurity()
> > > > 	- smack_inode_setsecurity()
> > > 
> > > What's the bug? The file descriptor returned by open(O_PATH) cannot be
> > > passed to read(2), write(2), fchmod(2), fchown(2), fgetxattr(2), mmap(2) etc.
> > > 
> > 
> > chmod(2), chown(2), getxattr(2), and setxattr(2) take a path, not a fd.
> > 
> 
> OK. Then, is the correct fix
> 
>   inode_lock(inode);
>   if (SOCKET_I(inode)->sk) {
>     // Can access SOCKET_I(sock)->sk->*
>   } else {
>     // Already close()d. Don't touch.
>   }
>   inode_unlock(inode);
> 
> thanks to
> 
>   commit 6d8c50dcb029872b ("socket: close race condition between sock_close() and sockfs_setattr()")
>   commit ff7b11aa481f682e ("net: socket: set sock->sk to NULL after calling proto_ops::release()")
> 
> changes?

inode_lock() is already held during security_path_chmod(),
security_path_chown(), and security_inode_setxattr().
So you can't just take it again.

- Eric

Tetsuo Handa Sept. 3, 2019, 6:52 a.m. UTC | #9

On 2019/07/07 11:50, James Morris wrote:
> On Sat, 6 Jul 2019, James Morris wrote:
> 
>> On Thu, 4 Jul 2019, Tetsuo Handa wrote:
>>
>>> Hello.
>>>
>>> Since it seems that Al has no comments, I'd like to send this patch to
>>> linux-next.git . What should I do? Do I need to set up a git tree?
>>
>> Yes, you can create one at github or similar.
> 
> Also notify Stephen Rothwell of the location of your -next branch, so it 
> gets pulled into his tree.
> 

I executed commands shown below. Since I'm not familiar with git management,
I want to use only master branch. Is this sequence correct?

# Upon initialization
git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
cd tomoyo-test1/
git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update upstream
git merge upstream/master
git push -u origin master

# When making changes
git remote update upstream
git merge upstream/master
git am 0001-tomoyo-Don-t-check-open-getattr-permission-on-socket.patch
git push -u origin master

Tetsuo Handa Sept. 13, 2019, 1:41 p.m. UTC | #10

Hello, Stephen Rothwell.

Can you add https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git#master
into the list for linux-next.git tree?

On 2019/09/03 15:52, Tetsuo Handa wrote:
> On 2019/07/07 11:50, James Morris wrote:
>> On Sat, 6 Jul 2019, James Morris wrote:
>>
>>> On Thu, 4 Jul 2019, Tetsuo Handa wrote:
>>>
>>>> Hello.
>>>>
>>>> Since it seems that Al has no comments, I'd like to send this patch to
>>>> linux-next.git . What should I do? Do I need to set up a git tree?
>>>
>>> Yes, you can create one at github or similar.
>>
>> Also notify Stephen Rothwell of the location of your -next branch, so it 
>> gets pulled into his tree.
>>
> 
> I executed commands shown below. Since I'm not familiar with git management,
> I want to use only master branch. Is this sequence correct?
> 
> # Upon initialization
> git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
> cd tomoyo-test1/
> git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git remote update upstream
> git merge upstream/master
> git push -u origin master
> 
> # When making changes
> git remote update upstream
> git merge upstream/master
> git am 0001-tomoyo-Don-t-check-open-getattr-permission-on-socket.patch
> git push -u origin master
>

Tetsuo Handa Oct. 2, 2019, 10:50 a.m. UTC | #11

On 2019/09/14 16:36, Stephen Rothwell wrote:
> Hi,
> 
> I am on vacation until after the merge window closes, so I will add it then.
> Please remind me if I don't.

Did you return from the vacation?

> 
> Cheers,
> Stephen Rothwell 
> 
> On 13 September 2019 2:41:24 pm GMT+01:00, Tetsuo Handa <[e-mail]> wrote:
>> Hello, Stephen Rothwell.
>>
>> Can you add https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git#master
>> into the list for linux-next.git tree?

Stephen Rothwell Oct. 2, 2019, 10:22 p.m. UTC | #12

Hi Tetsuo,

On Fri, 13 Sep 2019 22:41:24 +0900 Tetsuo Handa <[e-mail]> wrote:
>
> Can you add https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git#master
> into the list for linux-next.git tree?

Added from today.

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgement of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
        Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

Stephen Rothwell Oct. 2, 2019, 10:25 p.m. UTC | #13

Hi Tetsuo,

On Wed, 2 Oct 2019 19:50:48 +0900 Tetsuo Handa <[e-mail]> wrote:
>
> On 2019/09/14 16:36, Stephen Rothwell wrote:
> > 
> > I am on vacation until after the merge window closes, so I will add it then.
> > Please remind me if I don't.  
> 
> Did you return from the vacation?

I knew I'd missed one (but I have too much email :-().

I don't think the back merges of Linus' tree add anything useful to
your tree.  At this point it probably makes sense to just rebase the
single patch onto v5.4-rc1 and then not back merge Linus' tree at all
(unless some really complex conflict arises).

Tetsuo Handa Oct. 3, 2019, 9:59 a.m. UTC | #14

Hello.

On 2019/10/03 7:25, Stephen Rothwell wrote:
> Hi Tetsuo,
> 
> On Wed, 2 Oct 2019 19:50:48 +0900 Tetsuo Handa <[e-mail]> wrote:
>>
>> On 2019/09/14 16:36, Stephen Rothwell wrote:
>>>
>>> I am on vacation until after the merge window closes, so I will add it then.
>>> Please remind me if I don't.  
>>
>> Did you return from the vacation?
> 
> I knew I'd missed one (but I have too much email :-().

Thank you for adding my tree.

> 
> I don't think the back merges of Linus' tree add anything useful to
> your tree.  At this point it probably makes sense to just rebase the
> single patch onto v5.4-rc1 and then not back merge Linus' tree at all
> (unless some really complex conflict arises).
> 

This is my first time using persistent git tree.
I made my tree using the sequence shown below.

  # Upon initialization
  git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
  cd tomoyo-test1/
  git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  git remote update upstream
  git merge upstream/master
  git push -u origin master

According to
https://lkml.kernel.org/r/CAHk-=wg=[e-mail]
I should not try "git rebase" and "git merge" because I don't understand what they do. But
I guess I need to use "git merge" in order to update my tree before making changes.
Is the sequence shown below appropriate?

  # When making changes
  git remote update upstream
  git merge upstream/master
  edit files
  git commit
  git push -u origin master

Since I'm not familiar with git management, I want to use only master branch.
Do I need to make branches or another git tree for asking Linus to pull for linux.git ?

Tetsuo Handa Nov. 13, 2019, 1:49 p.m. UTC | #15

Hello, Andrew and James.

I have difficulty setting up environments for sending pull request to linux.git
(nobody around me knows Linux kernel maintainer's workflow at the command line level).
Can you pick up the following commit via mmotm or linux-security.git tree?

commit a5f9bda81cb4fa513f74707083b1eeee8735547f
Author: Tetsuo Handa <[e-mail]>
Date:   Sat Jun 22 13:14:26 2019 +0900

    tomoyo: Don't check open/getattr permission on sockets.

On 2019/10/03 18:59, Tetsuo Handa wrote:
> Hello.
> 
> On 2019/10/03 7:25, Stephen Rothwell wrote:
>> Hi Tetsuo,
>>
>> On Wed, 2 Oct 2019 19:50:48 +0900 Tetsuo Handa <[e-mail]> wrote:
>>>
>>> On 2019/09/14 16:36, Stephen Rothwell wrote:
>>>>
>>>> I am on vacation until after the merge window closes, so I will add it then.
>>>> Please remind me if I don't.  
>>>
>>> Did you return from the vacation?
>>
>> I knew I'd missed one (but I have too much email :-().
> 
> Thank you for adding my tree.
> 
>>
>> I don't think the back merges of Linus' tree add anything useful to
>> your tree.  At this point it probably makes sense to just rebase the
>> single patch onto v5.4-rc1 and then not back merge Linus' tree at all
>> (unless some really complex conflict arises).
>>
> 
> This is my first time using persistent git tree.
> I made my tree using the sequence shown below.
> 
>   # Upon initialization
>   git clone https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1.git
>   cd tomoyo-test1/
>   git remote add upstream git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>   git remote update upstream
>   git merge upstream/master
>   git push -u origin master
> 
> According to
> https://lkml.kernel.org/r/CAHk-=wg=[e-mail]
> I should not try "git rebase" and "git merge" because I don't understand what they do. But
> I guess I need to use "git merge" in order to update my tree before making changes.
> Is the sequence shown below appropriate?
> 
>   # When making changes
>   git remote update upstream
>   git merge upstream/master
>   edit files
>   git commit
>   git push -u origin master
> 
> Since I'm not familiar with git management, I want to use only master branch.
> Do I need to make branches or another git tree for asking Linus to pull for linux.git ?
>

James Morris Nov. 21, 2019, 7:21 a.m. UTC | #16

On Wed, 13 Nov 2019, Tetsuo Handa wrote:

> Hello, Andrew and James.
> 
> I have difficulty setting up environments for sending pull request to linux.git
> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
> Can you pick up the following commit via mmotm or linux-security.git tree?

Not sure if your fix is complete.

Are there other potential paths to trigger this via tomoyo_path_perm() ?

e.g. call unlink(2) on /proc/pid/fd/sockfd

Tetsuo Handa Nov. 21, 2019, 10:18 a.m. UTC | #17

On 2019/11/21 16:21, James Morris wrote:
> On Wed, 13 Nov 2019, Tetsuo Handa wrote:
> 
>> Hello, Andrew and James.
>>
>> I have difficulty setting up environments for sending pull request to linux.git
>> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
>> Can you pick up the following commit via mmotm or linux-security.git tree?
> 
> Not sure if your fix is complete.
> 
> Are there other potential paths to trigger this via tomoyo_path_perm() ?
> 
> e.g. call unlink(2) on /proc/pid/fd/sockfd

I think they are safe. For example, unlink(2) checks that
inode is valid before calling security_path_unlink().

        dentry = __lookup_hash(&last, path.dentry, lookup_flags);
        error = PTR_ERR(dentry);
        if (!IS_ERR(dentry)) {
                /* Why not before? Because we want correct error value */
                if (last.name[last.len])
                        goto slashes;
                inode = dentry->d_inode;
                if (d_is_negative(dentry))
                        goto slashes;
                ihold(inode);
                error = security_path_unlink(&path, dentry);
                if (error)
                        goto exit2;
                error = vfs_unlink(path.dentry->d_inode, dentry, &delegated_inode);
exit2:
                dput(dentry);
        }

Tetsuo Handa Nov. 21, 2019, 1:59 p.m. UTC | #18

On 2019/11/21 19:18, Tetsuo Handa wrote:
> On 2019/11/21 16:21, James Morris wrote:
>> On Wed, 13 Nov 2019, Tetsuo Handa wrote:
>>
>>> Hello, Andrew and James.
>>>
>>> I have difficulty setting up environments for sending pull request to linux.git
>>> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
>>> Can you pick up the following commit via mmotm or linux-security.git tree?
>>
>> Not sure if your fix is complete.
>>
>> Are there other potential paths to trigger this via tomoyo_path_perm() ?
>>
>> e.g. call unlink(2) on /proc/pid/fd/sockfd
> 
> I think they are safe. For example, unlink(2) checks that
> inode is valid before calling security_path_unlink().

Hmm, since unlink(2) locks parent's inode instead of inode to be removed itself,
there is indeed possibility that tomoyo_path_perm() races with __sock_release().
We need another patch...

Tetsuo Handa Dec. 4, 2019, 12:50 p.m. UTC | #19

On 2019/11/21 22:59, Tetsuo Handa wrote:
> On 2019/11/21 19:18, Tetsuo Handa wrote:
>> On 2019/11/21 16:21, James Morris wrote:
>>> On Wed, 13 Nov 2019, Tetsuo Handa wrote:
>>>
>>>> Hello, Andrew and James.
>>>>
>>>> I have difficulty setting up environments for sending pull request to linux.git
>>>> (nobody around me knows Linux kernel maintainer's workflow at the command line level).
>>>> Can you pick up the following commit via mmotm or linux-security.git tree?
>>>
>>> Not sure if your fix is complete.
>>>
>>> Are there other potential paths to trigger this via tomoyo_path_perm() ?
>>>
>>> e.g. call unlink(2) on /proc/pid/fd/sockfd
>>
>> I think they are safe. For example, unlink(2) checks that
>> inode is valid before calling security_path_unlink().
> 
> Hmm, since unlink(2) locks parent's inode instead of inode to be removed itself,
> there is indeed possibility that tomoyo_path_perm() races with __sock_release().
> We need another patch...
> 

I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?

commit c39593ab0500fcd6db290b311c120349927ddc04
Author: Tetsuo Handa <[e-mail]>
Date:   Mon Nov 25 10:46:51 2019 +0900

    tomoyo: Don't use nifty names on sockets.

James Morris Dec. 9, 2019, 9:37 p.m. UTC | #20

On Wed, 4 Dec 2019, Tetsuo Handa wrote:

> 
> I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?
> 
> commit c39593ab0500fcd6db290b311c120349927ddc04
> Author: Tetsuo Handa <[e-mail]>
> Date:   Mon Nov 25 10:46:51 2019 +0900
> 
>     tomoyo: Don't use nifty names on sockets.
> 

From where?

Please send a patch.

Tetsuo Handa Dec. 10, 2019, 10:21 a.m. UTC | #21

On 2019/12/10 6:37, James Morris wrote:
> On Wed, 4 Dec 2019, Tetsuo Handa wrote:
> 
>>
>> I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?
>>
>> commit c39593ab0500fcd6db290b311c120349927ddc04
>> Author: Tetsuo Handa <[e-mail]>
>> Date:   Mon Nov 25 10:46:51 2019 +0900
>>
>>     tomoyo: Don't use nifty names on sockets.
>>
> 
>>From where?
> 
> Please send a patch.
> 

Patch is at https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1 and was tested on linux-next.git .
But if you pick up c39593ab0500, what do I need to do (in order to avoid trying to apply the same
patch) ? Could you explain me (using command line) how I can send only c39593ab0500 to linux.git ?
https://osdn.net/projects/tomoyo/scm/git/tomoyo-test1/commits has only master branch.

c39593ab0500 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1
fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
19768fdc4025 Revert "printk: Monitor change of console loglevel."
07fca3f339d7 printk: Monitor change of console loglevel.
df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.
219d54332a09 (tag: v5.4, upstream/master) Linux 5.4

Stephen Rothwell Dec. 10, 2019, 11:02 p.m. UTC | #22

Hi Tetsuo,

On Tue, 10 Dec 2019 19:21:08 +0900 Tetsuo Handa <[e-mail]> wrote:
>
> On 2019/12/10 6:37, James Morris wrote:
> > On Wed, 4 Dec 2019, Tetsuo Handa wrote:
> >   
> >>
> >> I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?
> >>
> >> commit c39593ab0500fcd6db290b311c120349927ddc04
> >> Author: Tetsuo Handa <[e-mail]>
> >> Date:   Mon Nov 25 10:46:51 2019 +0900
> >>
> >>     tomoyo: Don't use nifty names on sockets.
> >>  
> >   
> >>From where?  
> > 
> > Please send a patch.
> >   
> 
> Patch is at https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1 and was tested on linux-next.git .
> But if you pick up c39593ab0500, what do I need to do (in order to avoid trying to apply the same
> patch) ? Could you explain me (using command line) how I can send only c39593ab0500 to linux.git ?
> https://osdn.net/projects/tomoyo/scm/git/tomoyo-test1/commits has only master branch.
> 
> c39593ab0500 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
> cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1
> fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
> 19768fdc4025 Revert "printk: Monitor change of console loglevel."
> 07fca3f339d7 printk: Monitor change of console loglevel.
> df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.
> 219d54332a09 (tag: v5.4, upstream/master) Linux 5.4

You should start by cleaning up your tree:

remove

fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
19768fdc4025 Revert "printk: Monitor change of console loglevel."
07fca3f339d7 printk: Monitor change of console loglevel.
df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.

since they end up cancelling each other out

cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1

only introduces these commits:

79c8ca578dbf Revert "printk: Monitor change of console loglevel."
23641a048089 printk: Monitor change of console loglevel.
a5f9bda81cb4 tomoyo: Don't check open/getattr permission on sockets.

and the first 2 above cancel each other out.

so you are left with these:

c39593ab0500 tomoyo: Don't use nifty names on sockets.
a5f9bda81cb4 tomoyo: Don't check open/getattr permission on sockets.

you should rebase these onto v5.5-rc1.

If you want James to just take the first of these, then the easiest way
is probably to just send him a patch that you generate using "git
format-patch" and then remove it from your tree.

Since there are no other changes to the only file affected by that
commit since v5.4, you could just do this:

$ git format-patch -o <some file> -1 c39593ab0500

and then <some file> to James using your email client.

Having done that, you should just do this (and forget the cleanups
above):

$ git checkout master
$ git remote update upstream
$ git reset --hard upstream/master
$ git cherry-pick a5f9bda81cb4
$ git push -f origin master

After that you will have a nice clean tree (based on Linus' tree) to
continue development on that just contains the one patch "tomoyo: Don't
check open/getattr permission on sockets."

If, however, you intend to only send patche via James tree, then you
should be using his tree
(git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
branch next-testing) as your upstream tree, not Linus' tree.  Then you
can ask him to merge your tree before the merge window opens during
each cycle.  You may want to rebase your tree on top of James tree
after he applies your patch from above.

Tetsuo Handa Dec. 11, 2019, 11:19 a.m. UTC | #23

Hello, Stephen Rothwell.

Thank you for the command line.

I was wondering why "git push -f" does not work. But I finally found
there is denyNonFastforwards option, and I was able to clean up.

$ git format-patch -1
0001-tomoyo-Don-t-use-nifty-names-on-sockets.patch
$ git reset --hard v5.5-rc1
$ git push -f origin master
$ git pull upstream master
$ git am 0001-tomoyo-Don-t-use-nifty-names-on-sockets.patch
$ git push origin master

On 2019/12/11 8:02, Stephen Rothwell wrote:
> Having done that, you should just do this (and forget the cleanups
> above):
> 
> $ git checkout master
> $ git remote update upstream
> $ git reset --hard upstream/master
> $ git cherry-pick a5f9bda81cb4
> $ git push -f origin master
> 
> After that you will have a nice clean tree (based on Linus' tree) to
> continue development on that just contains the one patch "tomoyo: Don't
> check open/getattr permission on sockets."

Now the history looks like below.

6f7c41374b62 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
6794862a16ef (upstream/master) Merge tag 'for-5.5-rc1-kconfig-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
184b8f7f91ca Merge tag 'printk-for-5.5-pr-warning-removal' of git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk
316eaf170252 Merge tag 'thermal-5.5-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux
78f926f72e43 btrfs: add Kconfig dependency for BLAKE2B
e42617b825f8 (tag: v5.5-rc1) Linux 5.5-rc1

> 
> If, however, you intend to only send patche via James tree, then you
> should be using his tree
> (git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
> branch next-testing) as your upstream tree, not Linus' tree.  Then you
> can ask him to merge your tree before the merge window opens during
> each cycle.  You may want to rebase your tree on top of James tree
> after he applies your patch from above.
> 

I was previously using linux-security.git . But after experiencing confusion of
linux-security.git management, LSM users (including me) were suggested to have
their own git tree and were suggested to directly send patches to Linus.
And I am currently experiencing confusion of my tree management (because I have
never maintained a tree for "git push" purpose)...

Next step is how to create a pull request. If I recall correctly, I need a
GPG key for signing my request? I have a GPG key but I have never attended
key signing party.

Patch

diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..8ea3f5d 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -126,6 +126,9 @@  static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
  */
 static int tomoyo_inode_getattr(const struct path *path)
 {
+	/* It is not safe to call tomoyo_get_socket_name(). */
+	if (S_ISSOCK(d_inode(path->dentry)->i_mode))
+		return 0;
 	return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
 }
 
@@ -316,6 +319,9 @@  static int tomoyo_file_open(struct file *f)
 	/* Don't check read permission here if called from do_execve(). */
 	if (current->in_execve)
 		return 0;
+	/* Sockets can't be opened by open(). */
+	if (S_ISSOCK(file_inode(f)->i_mode))
+		return 0;
 	return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
 					    f->f_flags);
 }