* [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open() [not found] <20200618012600.608744-1-sashal@kernel.org> @ 2020-06-18 1:25 ` Sasha Levin 2020-06-23 15:25 ` Naresh Kamboju 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2020-06-18 1:25 UTC (permalink / raw) To: linux-kernel, stable; +Cc: Miklos Szeredi, Sasha Levin, linux-unionfs From: Miklos Szeredi <mszeredi@redhat.com> [ Upstream commit 56230d956739b9cb1cbde439d76227d77979a04d ] Check permission before opening a real file. ovl_path_open() is used by readdir and copy-up routines. ovl_permission() theoretically already checked copy up permissions, but it doesn't hurt to re-do these checks during the actual copy-up. For directory reading ovl_permission() only checks access to topmost underlying layer. Readdir on a merged directory accesses layers below the topmost one as well. Permission wasn't checked for these layers. Note: modifying ovl_permission() to perform this check would be far more complex and hence more bug prone. The result is less precise permissions returned in access(2). If this turns out to be an issue, we can revisit this bug. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> Signed-off-by: Sasha Levin <sashal@kernel.org> --- fs/overlayfs/util.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index afdc2533ce74..76d6610767f6 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -307,7 +307,32 @@ bool ovl_is_whiteout(struct dentry *dentry) struct file *ovl_path_open(struct path *path, int flags) { - return dentry_open(path, flags | O_NOATIME, current_cred()); + struct inode *inode = d_inode(path->dentry); + int err, acc_mode; + + if (flags & ~(O_ACCMODE | O_LARGEFILE)) + BUG(); + + switch (flags & O_ACCMODE) { + case O_RDONLY: + acc_mode = MAY_READ; + break; + case O_WRONLY: + acc_mode = MAY_WRITE; + break; + default: + BUG(); + } + + err = inode_permission(inode, acc_mode | MAY_OPEN); + if (err) + return ERR_PTR(err); + + /* O_NOATIME is an optimization, don't fail if not permitted */ + if (inode_owner_or_capable(inode)) + flags |= O_NOATIME; + + return dentry_open(path, flags, current_cred()); } int ovl_copy_up_start(struct dentry *dentry) -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open() 2020-06-18 1:25 ` [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open() Sasha Levin @ 2020-06-23 15:25 ` Naresh Kamboju 2020-06-23 17:16 ` Sasha Levin 0 siblings, 1 reply; 5+ messages in thread From: Naresh Kamboju @ 2020-06-23 15:25 UTC (permalink / raw) To: Sasha Levin Cc: open list, linux- stable, Miklos Szeredi, linux-unionfs, lkft-triage On Thu, 18 Jun 2020 at 07:18, Sasha Levin <sashal@kernel.org> wrote: > > From: Miklos Szeredi <mszeredi@redhat.com> > > [ Upstream commit 56230d956739b9cb1cbde439d76227d77979a04d ] > > Check permission before opening a real file. > > ovl_path_open() is used by readdir and copy-up routines. > > ovl_permission() theoretically already checked copy up permissions, but it > doesn't hurt to re-do these checks during the actual copy-up. > > For directory reading ovl_permission() only checks access to topmost > underlying layer. Readdir on a merged directory accesses layers below the > topmost one as well. Permission wasn't checked for these layers. > > Note: modifying ovl_permission() to perform this check would be far more > complex and hence more bug prone. The result is less precise permissions > returned in access(2). If this turns out to be an issue, we can revisit > this bug. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > Signed-off-by: Sasha Levin <sashal@kernel.org> > --- > fs/overlayfs/util.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > index afdc2533ce74..76d6610767f6 100644 > --- a/fs/overlayfs/util.c > +++ b/fs/overlayfs/util.c > @@ -307,7 +307,32 @@ bool ovl_is_whiteout(struct dentry *dentry) > > struct file *ovl_path_open(struct path *path, int flags) > { > - return dentry_open(path, flags | O_NOATIME, current_cred()); > + struct inode *inode = d_inode(path->dentry); > + int err, acc_mode; > + > + if (flags & ~(O_ACCMODE | O_LARGEFILE)) > + BUG(); > + > + switch (flags & O_ACCMODE) { > + case O_RDONLY: > + acc_mode = MAY_READ; > + break; > + case O_WRONLY: > + acc_mode = MAY_WRITE; > + break; > + default: > + BUG(); This BUG: triggered on stable-rc 5.7, 5.4, 4.19 and 4.14. steps to reproduce: - cd /opt/ltp - ./runltp -s execveat03 Test output: mke2fs 1.43.8 (1-Jan-2018) [ 47.739682] ------------[ cut here ]------------ [ 47.744317] kernel BUG at fs/overlayfs/util.c:314! [ 47.749117] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP [ 47.754608] Modules linked in: overlay rfkill crc32_ce crct10dif_ce fuse [ 47.761335] Process execveat03 (pid: 2880, stack limit = 0xffff00000ec60000) [ 47.768397] CPU: 3 PID: 2880 Comm: execveat03 Not tainted 4.14.186-rc1-00111-gb518002db397 #1 [ 47.776933] Hardware name: ARM Juno development board (r2) (DT) [ 47.782860] task: ffff8009546ade80 task.stack: ffff00000ec60000 [ 47.788819] pc : ovl_path_open+0xa8/0xb0 [overlay] [ 47.793641] lr : ovl_check_d_type_supported+0x38/0xf0 [overlay] [ 47.799567] sp : ffff00000ec63ba0 pstate : 40000145 [ 47.804449] x29: ffff00000ec63ba0 x28: 0000000000000000 [ 47.809770] x27: ffff800955bfb710 x26: ffff800955bfb700 [ 47.815091] x25: ffff00000ec63ce0 x24: 0000000000000000 [ 47.820412] x23: ffff00000ec63cd0 x22: 0000000000000001 [ 47.825733] x21: ffff8009509609b8 x20: ffff00000ec63ce0 [ 47.831054] x19: 0000000000004000 x18: 0000000000000000 [ 47.836375] x17: 0000000000000000 x16: ffff0000080d1800 [ 47.841696] x15: 095a041701101c00 x14: ff00000000000000 [ 47.847017] x13: 0000000000000000 x12: 000000000000000b [ 47.852338] x11: 0101010101010101 x10: ffff800950960b40 [ 47.857659] x9 : 7f7f7f7f7f7f7f7f x8 : 6f2d6c6473727872 [ 47.862980] x7 : 001c100117045a09 x6 : 095a041701101c00 [ 47.868301] x5 : 0000000000000000 x4 : 0000000000000000 [ 47.873622] x3 : 0000000000000000 x2 : ffff000000b7ec88 [ 47.878943] x1 : ffff800953f78180 x0 : 0000000000004000 [ 47.884264] Call trace: [ 47.886736] ovl_path_open+0xa8/0xb0 [overlay] [ 47.891209] ovl_check_d_type_supported+0x38/0xf0 [overlay] [ 47.896812] ovl_fill_super+0x540/0xc98 [overlay] [ 47.901528] mount_nodev+0x4c/0xa8 [ 47.904954] ovl_mount+0x14/0x28 [overlay] [ 47.909056] mount_fs+0x54/0x188 [ 47.912289] vfs_kern_mount.part.0+0x4c/0x118 [ 47.916651] do_mount+0x1cc/0xbe0 [ 47.919970] compat_SyS_mount+0xb0/0x1b8 [ 47.923899] __sys_trace_return+0x0/0x4 [ 47.927742] Code: f94013f5 a8c37bfd d65f03c0 d4210000 (d4210000) [ 47.933845] ---[ end trace 1b32b515dde8d9db ]--- Test link, https://lkft.validation.linaro.org/scheduler/job/1517781#L1266 metadata: git branch: linux-4.14.y git repo: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git git commit: b518002db397b51173a3e17045bfb3ff0e1aa0ed kernel-config: https://builds.tuxbuild.com/B_xCv6-0v8npcEVw6hA_ZQ/kernel.config -- Linaro LKFT https://lkft.linaro.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open() 2020-06-23 15:25 ` Naresh Kamboju @ 2020-06-23 17:16 ` Sasha Levin 2020-06-23 18:28 ` Naresh Kamboju 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2020-06-23 17:16 UTC (permalink / raw) To: Naresh Kamboju Cc: open list, linux- stable, Miklos Szeredi, linux-unionfs, lkft-triage On Tue, Jun 23, 2020 at 08:55:38PM +0530, Naresh Kamboju wrote: >On Thu, 18 Jun 2020 at 07:18, Sasha Levin <sashal@kernel.org> wrote: >> >> From: Miklos Szeredi <mszeredi@redhat.com> >> >> [ Upstream commit 56230d956739b9cb1cbde439d76227d77979a04d ] >> >> Check permission before opening a real file. >> >> ovl_path_open() is used by readdir and copy-up routines. >> >> ovl_permission() theoretically already checked copy up permissions, but it >> doesn't hurt to re-do these checks during the actual copy-up. >> >> For directory reading ovl_permission() only checks access to topmost >> underlying layer. Readdir on a merged directory accesses layers below the >> topmost one as well. Permission wasn't checked for these layers. >> >> Note: modifying ovl_permission() to perform this check would be far more >> complex and hence more bug prone. The result is less precise permissions >> returned in access(2). If this turns out to be an issue, we can revisit >> this bug. >> >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> >> Signed-off-by: Sasha Levin <sashal@kernel.org> >> --- >> fs/overlayfs/util.c | 27 ++++++++++++++++++++++++++- >> 1 file changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c >> index afdc2533ce74..76d6610767f6 100644 >> --- a/fs/overlayfs/util.c >> +++ b/fs/overlayfs/util.c >> @@ -307,7 +307,32 @@ bool ovl_is_whiteout(struct dentry *dentry) >> >> struct file *ovl_path_open(struct path *path, int flags) >> { >> - return dentry_open(path, flags | O_NOATIME, current_cred()); >> + struct inode *inode = d_inode(path->dentry); >> + int err, acc_mode; >> + >> + if (flags & ~(O_ACCMODE | O_LARGEFILE)) >> + BUG(); >> + >> + switch (flags & O_ACCMODE) { >> + case O_RDONLY: >> + acc_mode = MAY_READ; >> + break; >> + case O_WRONLY: >> + acc_mode = MAY_WRITE; >> + break; >> + default: >> + BUG(); > >This BUG: triggered on stable-rc 5.7, 5.4, 4.19 and 4.14. > >steps to reproduce: > - cd /opt/ltp > - ./runltp -s execveat03 Yup, that patch has been dropped, thanks for testing! -- Thanks, Sasha ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open() 2020-06-23 17:16 ` Sasha Levin @ 2020-06-23 18:28 ` Naresh Kamboju 2020-06-23 18:59 ` Amir Goldstein 0 siblings, 1 reply; 5+ messages in thread From: Naresh Kamboju @ 2020-06-23 18:28 UTC (permalink / raw) To: Sasha Levin Cc: open list, linux- stable, Miklos Szeredi, linux-unionfs, lkft-triage On Tue, 23 Jun 2020 at 22:46, Sasha Levin <sashal@kernel.org> wrote: > > On Tue, Jun 23, 2020 at 08:55:38PM +0530, Naresh Kamboju wrote: > >On Thu, 18 Jun 2020 at 07:18, Sasha Levin <sashal@kernel.org> wrote: > >> > >> From: Miklos Szeredi <mszeredi@redhat.com> > >> > >> [ Upstream commit 56230d956739b9cb1cbde439d76227d77979a04d ] > >> > >> Check permission before opening a real file. > >> > >> ovl_path_open() is used by readdir and copy-up routines. > >> > >> ovl_permission() theoretically already checked copy up permissions, but it > >> doesn't hurt to re-do these checks during the actual copy-up. > >> > >> For directory reading ovl_permission() only checks access to topmost > >> underlying layer. Readdir on a merged directory accesses layers below the > >> topmost one as well. Permission wasn't checked for these layers. > >> > >> Note: modifying ovl_permission() to perform this check would be far more > >> complex and hence more bug prone. The result is less precise permissions > >> returned in access(2). If this turns out to be an issue, we can revisit > >> this bug. > >> > >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > >> Signed-off-by: Sasha Levin <sashal@kernel.org> > >> --- > >> fs/overlayfs/util.c | 27 ++++++++++++++++++++++++++- > >> 1 file changed, 26 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > >> index afdc2533ce74..76d6610767f6 100644 > >> --- a/fs/overlayfs/util.c > >> +++ b/fs/overlayfs/util.c > >> @@ -307,7 +307,32 @@ bool ovl_is_whiteout(struct dentry *dentry) > >> > >> struct file *ovl_path_open(struct path *path, int flags) > >> { > >> - return dentry_open(path, flags | O_NOATIME, current_cred()); > >> + struct inode *inode = d_inode(path->dentry); > >> + int err, acc_mode; > >> + > >> + if (flags & ~(O_ACCMODE | O_LARGEFILE)) > >> + BUG(); > >> + > >> + switch (flags & O_ACCMODE) { > >> + case O_RDONLY: > >> + acc_mode = MAY_READ; > >> + break; > >> + case O_WRONLY: > >> + acc_mode = MAY_WRITE; > >> + break; > >> + default: > >> + BUG(); > > > >This BUG: triggered on stable-rc 5.7, 5.4, 4.19 and 4.14. > > > >steps to reproduce: > > - cd /opt/ltp > > - ./runltp -s execveat03 > > Yup, that patch has been dropped, thanks for testing! After reverting this patch I see these messages while testing LTP execveat03. [ 87.931537] overlayfs: upper fs does not support RENAME_WHITEOUT. [ 87.937693] overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.> - Naresh > -- > Thanks, > Sasha ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open() 2020-06-23 18:28 ` Naresh Kamboju @ 2020-06-23 18:59 ` Amir Goldstein 0 siblings, 0 replies; 5+ messages in thread From: Amir Goldstein @ 2020-06-23 18:59 UTC (permalink / raw) To: Naresh Kamboju Cc: Sasha Levin, open list, linux- stable, Miklos Szeredi, overlayfs, lkft-triage On Tue, Jun 23, 2020 at 9:28 PM Naresh Kamboju <naresh.kamboju@linaro.org> wrote: > > On Tue, 23 Jun 2020 at 22:46, Sasha Levin <sashal@kernel.org> wrote: > > > > On Tue, Jun 23, 2020 at 08:55:38PM +0530, Naresh Kamboju wrote: > > >On Thu, 18 Jun 2020 at 07:18, Sasha Levin <sashal@kernel.org> wrote: > > >> > > >> From: Miklos Szeredi <mszeredi@redhat.com> > > >> > > >> [ Upstream commit 56230d956739b9cb1cbde439d76227d77979a04d ] > > >> > > >> Check permission before opening a real file. > > >> > > >> ovl_path_open() is used by readdir and copy-up routines. > > >> > > >> ovl_permission() theoretically already checked copy up permissions, but it > > >> doesn't hurt to re-do these checks during the actual copy-up. > > >> > > >> For directory reading ovl_permission() only checks access to topmost > > >> underlying layer. Readdir on a merged directory accesses layers below the > > >> topmost one as well. Permission wasn't checked for these layers. > > >> > > >> Note: modifying ovl_permission() to perform this check would be far more > > >> complex and hence more bug prone. The result is less precise permissions > > >> returned in access(2). If this turns out to be an issue, we can revisit > > >> this bug. > > >> > > >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > >> Signed-off-by: Sasha Levin <sashal@kernel.org> > > >> --- > > >> fs/overlayfs/util.c | 27 ++++++++++++++++++++++++++- > > >> 1 file changed, 26 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > > >> index afdc2533ce74..76d6610767f6 100644 > > >> --- a/fs/overlayfs/util.c > > >> +++ b/fs/overlayfs/util.c > > >> @@ -307,7 +307,32 @@ bool ovl_is_whiteout(struct dentry *dentry) > > >> > > >> struct file *ovl_path_open(struct path *path, int flags) > > >> { > > >> - return dentry_open(path, flags | O_NOATIME, current_cred()); > > >> + struct inode *inode = d_inode(path->dentry); > > >> + int err, acc_mode; > > >> + > > >> + if (flags & ~(O_ACCMODE | O_LARGEFILE)) > > >> + BUG(); > > >> + > > >> + switch (flags & O_ACCMODE) { > > >> + case O_RDONLY: > > >> + acc_mode = MAY_READ; > > >> + break; > > >> + case O_WRONLY: > > >> + acc_mode = MAY_WRITE; > > >> + break; > > >> + default: > > >> + BUG(); > > > > > >This BUG: triggered on stable-rc 5.7, 5.4, 4.19 and 4.14. > > > > > >steps to reproduce: > > > - cd /opt/ltp > > > - ./runltp -s execveat03 > > > > Yup, that patch has been dropped, thanks for testing! > > After reverting this patch I see these messages while testing LTP execveat03. > [ 87.931537] overlayfs: upper fs does not support RENAME_WHITEOUT. This warning is new. > [ 87.937693] overlayfs: upper fs does not support xattr, falling back > to index=off and metacopy=off.> This one is pretty old. They will show up when testing with a filesystem that is suboptimal for upper fs. The warning is stating a fact. The suboptimal behavior with those filesystems has been there since the beginning. Thanks, Amir. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-23 19:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200618012600.608744-1-sashal@kernel.org>
2020-06-18 1:25 ` [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open() Sasha Levin
2020-06-23 15:25 ` Naresh Kamboju
2020-06-23 17:16 ` Sasha Levin
2020-06-23 18:28 ` Naresh Kamboju
2020-06-23 18:59 ` Amir Goldstein
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).