[Xenial][Bionic][SRU][PATCH 0/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[Xenial][Bionic][SRU][PATCH 0/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

Po-Hsu Lin (Sam)
BugLink: https://bugs.launchpad.net/bugs/1786729

== Justification ==
The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
("Introduce v3 namespaced file capabilities"), should use
d_find_any_alias() instead of d_find_alias() do handle unhashed dentry
correctly. This is needed, for example, if execveat() is called with an
open but unlinked overlayfs file, because overlayfs unhashes dentry on
unlink.
This is a regression of real life application, first reported at
https://www.spinics.net/lists/linux-unionfs/msg05363.html

With the execveat03 test in the LTP test suite on an affected kernel, it will fail with:
<<<test_start>>>
tag=execveat03 stime=1534135632
cmdline="execveat03"
contacts=""
analysis=exit
<<<test_output>>>
incrementing stop
tst_test.c:1017: INFO: Timeout per run is 0h 05m 00s
execveat03.c:70: FAIL: execveat() returned unexpected errno: EINVAL

Summary:
passed 0
failed 1
skipped 0
warnings 0

== Fix ==
355139a8 (cap_inode_getsecurity: use d_find_any_alias() instead of
 d_find_alias())

It can be cherry-picked for Bionic, but it needs to be backported to Xenial along with the logic when we backport 8db6c34f1dbc (bug 1778286).

The test kernel for Xenial / Bionic could be found here:
http://people.canonical.com/~phlin/kernel/lp-1786729-execveat03/

This patch has already been cherry-picked into Cosmic and Unstable.

== Regression Potential ==
Low, this patch just uses a correct function to handle unhashed dentry, and it's been applied in both upstream and our newer kernel.

== Test Case ==
Run the reproducer in the commit message, or,
run the execveat03 test in ubuntu_ltp_syscalls test suite. And it will pass with the patched kernel.



Eddie.Horng (1):
  cap_inode_getsecurity: use d_find_any_alias() instead of
    d_find_alias()

 security/commoncap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Xenial][SRU][PATCH 1/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

Po-Hsu Lin (Sam)
From: "Eddie.Horng" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1786729

The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
("Introduce v3 namespaced file capabilities"), should use
d_find_any_alias() instead of d_find_alias() do handle unhashed dentry
correctly. This is needed, for example, if execveat() is called with an
open but unlinked overlayfs file, because overlayfs unhashes dentry on
unlink.
This is a regression of real life application, first reported at
https://www.spinics.net/lists/linux-unionfs/msg05363.html

Below reproducer and setup can reproduce the case.
  const char* exec="echo";
  const char *newargv[] = { "echo", "hello", NULL};
  const char *newenviron[] = { NULL };
  int fd, err;

  fd = open(exec, O_PATH);
  unlink(exec);
  err = syscall(322/*SYS_execveat*/, fd, "", newargv, newenviron,
AT_EMPTY_PATH);
  if(err<0)
    fprintf(stderr, "execveat: %s\n", strerror(errno));

gcc compile into ~/test/a.out
mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w
none /mnt/m
cd /mnt/m
cp /bin/echo .
~/test/a.out

Expected result:
hello
Actually result:
execveat: Invalid argument
dmesg:
Invalid argument reading file caps for /dev/fd/3

The 2nd reproducer and setup emulates similar case but for
regular filesystem:
  const char* exec="echo";
  int fd, err;
  char buf[256];

  fd = open(exec, O_RDONLY);
  unlink(exec);
  err = fgetxattr(fd, "security.capability", buf, 256);
  if(err<0)
    fprintf(stderr, "fgetxattr: %s\n", strerror(errno));

gcc compile into ~/test_fgetxattr

cd /tmp
cp /bin/echo .
~/test_fgetxattr

Result:
fgetxattr: Invalid argument

On regular filesystem, for example, ext4 read xattr from
disk and return to execveat(), will not trigger this issue, however,
the overlay attr handler pass real dentry to vfs_getxattr() will.
This reproducer calls fgetxattr() with an unlinked fd, involkes
vfs_getxattr() then reproduced the case that d_find_alias() in
cap_inode_getsecurity() can't find the unlinked dentry.

Suggested-by: Amir Goldstein <[hidden email]>
Acked-by: Amir Goldstein <[hidden email]>
Acked-by: Serge E. Hallyn <[hidden email]>
Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
Cc: <[hidden email]> # v4.14
Signed-off-by: Eddie Horng <[hidden email]>
Signed-off-by: Eric W. Biederman <[hidden email]>
(backported from commit 355139a8dba446cc11a424cddbf7afebc3041ba1)
Signed-off-by: Po-Hsu Lin <[hidden email]>
---
 security/commoncap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 814324f..f2b35c7 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -399,7 +399,7 @@ int cap_inode_getsecurity(const struct inode *inode, const char *name,
  if (strcmp(name, "capability") != 0)
  return -EOPNOTSUPP;
 
- dentry = d_find_alias((struct inode *)inode);
+ dentry = d_find_any_alias((struct inode *)inode);
  if (!dentry)
  return -EINVAL;
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Bionic][SRU][PATCH][PATCH 1/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

Po-Hsu Lin (Sam)
In reply to this post by Po-Hsu Lin (Sam)
From: "Eddie.Horng" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1786729

The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
("Introduce v3 namespaced file capabilities"), should use
d_find_any_alias() instead of d_find_alias() do handle unhashed dentry
correctly. This is needed, for example, if execveat() is called with an
open but unlinked overlayfs file, because overlayfs unhashes dentry on
unlink.
This is a regression of real life application, first reported at
https://www.spinics.net/lists/linux-unionfs/msg05363.html

Below reproducer and setup can reproduce the case.
  const char* exec="echo";
  const char *newargv[] = { "echo", "hello", NULL};
  const char *newenviron[] = { NULL };
  int fd, err;

  fd = open(exec, O_PATH);
  unlink(exec);
  err = syscall(322/*SYS_execveat*/, fd, "", newargv, newenviron,
AT_EMPTY_PATH);
  if(err<0)
    fprintf(stderr, "execveat: %s\n", strerror(errno));

gcc compile into ~/test/a.out
mount -t overlay -orw,lowerdir=/mnt/l,upperdir=/mnt/u,workdir=/mnt/w
none /mnt/m
cd /mnt/m
cp /bin/echo .
~/test/a.out

Expected result:
hello
Actually result:
execveat: Invalid argument
dmesg:
Invalid argument reading file caps for /dev/fd/3

The 2nd reproducer and setup emulates similar case but for
regular filesystem:
  const char* exec="echo";
  int fd, err;
  char buf[256];

  fd = open(exec, O_RDONLY);
  unlink(exec);
  err = fgetxattr(fd, "security.capability", buf, 256);
  if(err<0)
    fprintf(stderr, "fgetxattr: %s\n", strerror(errno));

gcc compile into ~/test_fgetxattr

cd /tmp
cp /bin/echo .
~/test_fgetxattr

Result:
fgetxattr: Invalid argument

On regular filesystem, for example, ext4 read xattr from
disk and return to execveat(), will not trigger this issue, however,
the overlay attr handler pass real dentry to vfs_getxattr() will.
This reproducer calls fgetxattr() with an unlinked fd, involkes
vfs_getxattr() then reproduced the case that d_find_alias() in
cap_inode_getsecurity() can't find the unlinked dentry.

Suggested-by: Amir Goldstein <[hidden email]>
Acked-by: Amir Goldstein <[hidden email]>
Acked-by: Serge E. Hallyn <[hidden email]>
Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
Cc: <[hidden email]> # v4.14
Signed-off-by: Eddie Horng <[hidden email]>
Signed-off-by: Eric W. Biederman <[hidden email]>
(cherry picked from commit 355139a8dba446cc11a424cddbf7afebc3041ba1)
Signed-off-by: Po-Hsu Lin <[hidden email]>
---
 security/commoncap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 5c83db3..4ec20be 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -388,7 +388,7 @@ int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer,
  if (strcmp(name, "capability") != 0)
  return -EOPNOTSUPP;
 
- dentry = d_find_alias(inode);
+ dentry = d_find_any_alias(inode);
  if (!dentry)
  return -EINVAL;
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/Cmnt: [Xenial][Bionic][SRU][PATCH 0/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

Stefan Bader-2
In reply to this post by Po-Hsu Lin (Sam)
On 24.10.18 08:54, Po-Hsu Lin wrote:

> BugLink: https://bugs.launchpad.net/bugs/1786729
>
> == Justification ==
> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
> ("Introduce v3 namespaced file capabilities"), should use
> d_find_any_alias() instead of d_find_alias() do handle unhashed dentry
> correctly. This is needed, for example, if execveat() is called with an
> open but unlinked overlayfs file, because overlayfs unhashes dentry on
> unlink.
> This is a regression of real life application, first reported at
> https://www.spinics.net/lists/linux-unionfs/msg05363.html
>
> With the execveat03 test in the LTP test suite on an affected kernel, it will fail with:
> <<<test_start>>>
> tag=execveat03 stime=1534135632
> cmdline="execveat03"
> contacts=""
> analysis=exit
> <<<test_output>>>
> incrementing stop
> tst_test.c:1017: INFO: Timeout per run is 0h 05m 00s
> execveat03.c:70: FAIL: execveat() returned unexpected errno: EINVAL
>
> Summary:
> passed 0
> failed 1
> skipped 0
> warnings 0
>
> == Fix ==
> 355139a8 (cap_inode_getsecurity: use d_find_any_alias() instead of
>  d_find_alias())
>
> It can be cherry-picked for Bionic, but it needs to be backported to Xenial along with the logic when we backport 8db6c34f1dbc (bug 1778286).
>
> The test kernel for Xenial / Bionic could be found here:
> http://people.canonical.com/~phlin/kernel/lp-1786729-execveat03/
>
> This patch has already been cherry-picked into Cosmic and Unstable.
>
> == Regression Potential ==
> Low, this patch just uses a correct function to handle unhashed dentry, and it's been applied in both upstream and our newer kernel.
>
> == Test Case ==
> Run the reproducer in the commit message, or,
> run the execveat03 test in ubuntu_ltp_syscalls test suite. And it will pass with the patched kernel.
>
>
>
> Eddie.Horng (1):
>   cap_inode_getsecurity: use d_find_any_alias() instead of
>     d_find_alias()
>
>  security/commoncap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Just wondering about the state of linux-aws in the related bug report. I think
there are probably no guidelines on it but I would say if something is not
urgently needed for a derivative and also present in the master kernel, then I
would suggest to only keep a linux task. Or maybe I do not understand fully what
you tried to achieve.

-Stefan

Acked-by: Stefan Bader <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [Xenial][Bionic][SRU][PATCH 0/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

Kleber Sacilotto de Souza
In reply to this post by Po-Hsu Lin (Sam)
On 10/24/18 08:54, Po-Hsu Lin wrote:

> BugLink: https://bugs.launchpad.net/bugs/1786729
>
> == Justification ==
> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
> ("Introduce v3 namespaced file capabilities"), should use
> d_find_any_alias() instead of d_find_alias() do handle unhashed dentry
> correctly. This is needed, for example, if execveat() is called with an
> open but unlinked overlayfs file, because overlayfs unhashes dentry on
> unlink.
> This is a regression of real life application, first reported at
> https://www.spinics.net/lists/linux-unionfs/msg05363.html
>
> With the execveat03 test in the LTP test suite on an affected kernel, it will fail with:
> <<<test_start>>>
> tag=execveat03 stime=1534135632
> cmdline="execveat03"
> contacts=""
> analysis=exit
> <<<test_output>>>
> incrementing stop
> tst_test.c:1017: INFO: Timeout per run is 0h 05m 00s
> execveat03.c:70: FAIL: execveat() returned unexpected errno: EINVAL
>
> Summary:
> passed 0
> failed 1
> skipped 0
> warnings 0
>
> == Fix ==
> 355139a8 (cap_inode_getsecurity: use d_find_any_alias() instead of
>  d_find_alias())
>
> It can be cherry-picked for Bionic, but it needs to be backported to Xenial along with the logic when we backport 8db6c34f1dbc (bug 1778286).
>
> The test kernel for Xenial / Bionic could be found here:
> http://people.canonical.com/~phlin/kernel/lp-1786729-execveat03/
>
> This patch has already been cherry-picked into Cosmic and Unstable.
>
> == Regression Potential ==
> Low, this patch just uses a correct function to handle unhashed dentry, and it's been applied in both upstream and our newer kernel.
>
> == Test Case ==
> Run the reproducer in the commit message, or,
> run the execveat03 test in ubuntu_ltp_syscalls test suite. And it will pass with the patched kernel.
>
>
>
> Eddie.Horng (1):
>   cap_inode_getsecurity: use d_find_any_alias() instead of
>     d_find_alias()
>
>  security/commoncap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Acked-by: Kleber Sacilotto de Souza <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [Xenial][Bionic][SRU][PATCH 0/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

Khaled Elmously
In reply to this post by Po-Hsu Lin (Sam)
On 2018-10-24 14:54:48 , Po-Hsu Lin wrote:

> BugLink: https://bugs.launchpad.net/bugs/1786729
>
> == Justification ==
> The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
> ("Introduce v3 namespaced file capabilities"), should use
> d_find_any_alias() instead of d_find_alias() do handle unhashed dentry
> correctly. This is needed, for example, if execveat() is called with an
> open but unlinked overlayfs file, because overlayfs unhashes dentry on
> unlink.
> This is a regression of real life application, first reported at
> https://www.spinics.net/lists/linux-unionfs/msg05363.html
>
> With the execveat03 test in the LTP test suite on an affected kernel, it will fail with:
> <<<test_start>>>
> tag=execveat03 stime=1534135632
> cmdline="execveat03"
> contacts=""
> analysis=exit
> <<<test_output>>>
> incrementing stop
> tst_test.c:1017: INFO: Timeout per run is 0h 05m 00s
> execveat03.c:70: FAIL: execveat() returned unexpected errno: EINVAL
>
> Summary:
> passed 0
> failed 1
> skipped 0
> warnings 0
>
> == Fix ==
> 355139a8 (cap_inode_getsecurity: use d_find_any_alias() instead of
>  d_find_alias())
>
> It can be cherry-picked for Bionic, but it needs to be backported to Xenial along with the logic when we backport 8db6c34f1dbc (bug 1778286).
>
> The test kernel for Xenial / Bionic could be found here:
> http://people.canonical.com/~phlin/kernel/lp-1786729-execveat03/
>
> This patch has already been cherry-picked into Cosmic and Unstable.
>
> == Regression Potential ==
> Low, this patch just uses a correct function to handle unhashed dentry, and it's been applied in both upstream and our newer kernel.
>
> == Test Case ==
> Run the reproducer in the commit message, or,
> run the execveat03 test in ubuntu_ltp_syscalls test suite. And it will pass with the patched kernel.
>
>
>
> Eddie.Horng (1):
>   cap_inode_getsecurity: use d_find_any_alias() instead of
>     d_find_alias()
>
>  security/commoncap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --
> 2.7.4
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: ACK/Cmnt: [Xenial][Bionic][SRU][PATCH 0/1] cap_inode_getsecurity: use d_find_any_alias() instead of d_find_alias()

Po-Hsu Lin (Sam)
In reply to this post by Stefan Bader-2
On Mon, Nov 5, 2018 at 10:24 PM Stefan Bader <[hidden email]> wrote:

>
> On 24.10.18 08:54, Po-Hsu Lin wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1786729
> >
> > == Justification ==
> > The code in cap_inode_getsecurity(), introduced by commit 8db6c34f1dbc
> > ("Introduce v3 namespaced file capabilities"), should use
> > d_find_any_alias() instead of d_find_alias() do handle unhashed dentry
> > correctly. This is needed, for example, if execveat() is called with an
> > open but unlinked overlayfs file, because overlayfs unhashes dentry on
> > unlink.
> > This is a regression of real life application, first reported at
> > https://www.spinics.net/lists/linux-unionfs/msg05363.html
> >
> > With the execveat03 test in the LTP test suite on an affected kernel, it will fail with:
> > <<<test_start>>>
> > tag=execveat03 stime=1534135632
> > cmdline="execveat03"
> > contacts=""
> > analysis=exit
> > <<<test_output>>>
> > incrementing stop
> > tst_test.c:1017: INFO: Timeout per run is 0h 05m 00s
> > execveat03.c:70: FAIL: execveat() returned unexpected errno: EINVAL
> >
> > Summary:
> > passed 0
> > failed 1
> > skipped 0
> > warnings 0
> >
> > == Fix ==
> > 355139a8 (cap_inode_getsecurity: use d_find_any_alias() instead of
> >  d_find_alias())
> >
> > It can be cherry-picked for Bionic, but it needs to be backported to Xenial along with the logic when we backport 8db6c34f1dbc (bug 1778286).
> >
> > The test kernel for Xenial / Bionic could be found here:
> > http://people.canonical.com/~phlin/kernel/lp-1786729-execveat03/
> >
> > This patch has already been cherry-picked into Cosmic and Unstable.
> >
> > == Regression Potential ==
> > Low, this patch just uses a correct function to handle unhashed dentry, and it's been applied in both upstream and our newer kernel.
> >
> > == Test Case ==
> > Run the reproducer in the commit message, or,
> > run the execveat03 test in ubuntu_ltp_syscalls test suite. And it will pass with the patched kernel.
> >
> >
> >
> > Eddie.Horng (1):
> >   cap_inode_getsecurity: use d_find_any_alias() instead of
> >     d_find_alias()
> >
> >  security/commoncap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Just wondering about the state of linux-aws in the related bug report. I think
> there are probably no guidelines on it but I would say if something is not
> urgently needed for a derivative and also present in the master kernel, then I
> would suggest to only keep a linux task. Or maybe I do not understand fully what
> you tried to achieve.
>
Thanks for the comment, yes we can just get this into the master kernel.

Cheers
Sam

> -Stefan
>
> Acked-by: Stefan Bader <[hidden email]>
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team