* [PATCH v10 3/5] overlayfs: add __get xattr method
From: Mark Salyzyn @ 2019-07-24 19:57 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Mark Salyzyn, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Amir Goldstein, Randy Dunlap,
Stephen Smalley, linux-unionfs, linux-doc
In-Reply-To: <20190724195719.218307-1-salyzyn@android.com>
Because of the overlayfs getxattr recursion, the incoming inode fails
to update the selinux sid resulting in avc denials being reported
against a target context of u:object_r:unlabeled:s0.
Solution is to add a _get xattr method that calls the __vfs_getxattr
handler so that the context can be read in, rather than being denied
with an -EACCES when vfs_getxattr handler is called.
Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
---
v10 - added to patch series
---
fs/overlayfs/inode.c | 15 +++++++++++++++
fs/overlayfs/overlayfs.h | 2 ++
fs/overlayfs/super.c | 18 ++++++++++++++++++
3 files changed, 35 insertions(+)
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 7663aeb85fa3..d3b53849615c 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -362,6 +362,21 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
return err;
}
+int __ovl_xattr_get(struct dentry *dentry, struct inode *inode,
+ const char *name, void *value, size_t size)
+{
+ ssize_t res;
+ const struct cred *old_cred;
+ struct dentry *realdentry =
+ ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
+
+ old_cred = ovl_override_creds(dentry->d_sb);
+ res = __vfs_getxattr(realdentry, d_inode(realdentry), name, value,
+ size);
+ ovl_revert_creds(old_cred);
+ return res;
+}
+
int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
void *value, size_t size)
{
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6934bcf030f0..73a02a263fbc 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -357,6 +357,8 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
const void *value, size_t size, int flags);
int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
void *value, size_t size);
+int __ovl_xattr_get(struct dentry *dentry, struct inode *inode,
+ const char *name, void *value, size_t size);
ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
struct posix_acl *ovl_get_acl(struct inode *inode, int type);
int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b368e2e102fa..82e1130de206 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -859,6 +859,14 @@ ovl_posix_acl_xattr_get(const struct xattr_handler *handler,
return ovl_xattr_get(dentry, inode, handler->name, buffer, size);
}
+static int __maybe_unused
+__ovl_posix_acl_xattr_get(const struct xattr_handler *handler,
+ struct dentry *dentry, struct inode *inode,
+ const char *name, void *buffer, size_t size)
+{
+ return __ovl_xattr_get(dentry, inode, handler->name, buffer, size);
+}
+
static int __maybe_unused
ovl_posix_acl_xattr_set(const struct xattr_handler *handler,
struct dentry *dentry, struct inode *inode,
@@ -939,6 +947,13 @@ static int ovl_other_xattr_get(const struct xattr_handler *handler,
return ovl_xattr_get(dentry, inode, name, buffer, size);
}
+static int __ovl_other_xattr_get(const struct xattr_handler *handler,
+ struct dentry *dentry, struct inode *inode,
+ const char *name, void *buffer, size_t size)
+{
+ return __ovl_xattr_get(dentry, inode, name, buffer, size);
+}
+
static int ovl_other_xattr_set(const struct xattr_handler *handler,
struct dentry *dentry, struct inode *inode,
const char *name, const void *value,
@@ -952,6 +967,7 @@ ovl_posix_acl_access_xattr_handler = {
.name = XATTR_NAME_POSIX_ACL_ACCESS,
.flags = ACL_TYPE_ACCESS,
.get = ovl_posix_acl_xattr_get,
+ .__get = __ovl_posix_acl_xattr_get,
.set = ovl_posix_acl_xattr_set,
};
@@ -960,6 +976,7 @@ ovl_posix_acl_default_xattr_handler = {
.name = XATTR_NAME_POSIX_ACL_DEFAULT,
.flags = ACL_TYPE_DEFAULT,
.get = ovl_posix_acl_xattr_get,
+ .__get = __ovl_posix_acl_xattr_get,
.set = ovl_posix_acl_xattr_set,
};
@@ -972,6 +989,7 @@ static const struct xattr_handler ovl_own_xattr_handler = {
static const struct xattr_handler ovl_other_xattr_handler = {
.prefix = "", /* catch all */
.get = ovl_other_xattr_get,
+ .__get = __ovl_other_xattr_get,
.set = ovl_other_xattr_set,
};
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v10 1/5] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh
From: Mark Salyzyn @ 2019-07-24 19:57 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Mark Salyzyn, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Amir Goldstein, Randy Dunlap,
Stephen Smalley, linux-unionfs, linux-doc
In-Reply-To: <20190724195719.218307-1-salyzyn@android.com>
Assumption never checked, should fail if the mounter creds are not
sufficient.
Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
---
v10:
- return NULL rather than ERR_PTR(-EPERM)
- did _not_ add it ovl_can_decode_fh() because of changes since last
review, suspect needs to be added to ovl_lower_uuid_ok()?
v8 + v9:
- rebase
v7:
- This time for realz
v6:
- rebase
v5:
- dependency of "overlayfs: override_creds=off option bypass creator_cred"
---
fs/overlayfs/namei.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e9717c2f7d45..9702f0d5309d 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -161,6 +161,9 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
return NULL;
+ if (!capable(CAP_DAC_READ_SEARCH))
+ return NULL;
+
bytes = (fh->len - offsetof(struct ovl_fh, fid));
real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
bytes >> 2, (int)fh->type,
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v10 0/2] overlayfs override_creds=off
From: Mark Salyzyn @ 2019-07-24 19:57 UTC (permalink / raw)
To: linux-kernel
Cc: kernel-team, Mark Salyzyn, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Amir Goldstein, Randy Dunlap,
Stephen Smalley, linux-unionfs, linux-doc
Patch series:
overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh
Add optional __get xattr method paired to __vfs_getxattr
overlayfs: add __get xattr method
overlayfs: internal getxattr operations without sepolicy checking
overlayfs: override_creds=off option bypass creator_cred
The first four patches address fundamental security issues that should
be solved regardless of the override_creds=off feature.
The fifth that adds the feature depends on these other fixes.
By default, all access to the upper, lower and work directories is the
recorded mounter's MAC and DAC credentials. The incoming accesses are
checked against the caller's credentials.
If the principles of least privilege are applied for sepolicy, the
mounter's credentials might not overlap the credentials of the caller's
when accessing the overlayfs filesystem. For example, a file that a
lower DAC privileged caller can execute, is MAC denied to the
generally higher DAC privileged mounter, to prevent an attack vector.
We add the option to turn off override_creds in the mount options; all
subsequent operations after mount on the filesystem will be only the
caller's credentials. The module boolean parameter and mount option
override_creds is also added as a presence check for this "feature",
existence of /sys/module/overlay/parameters/overlay_creds
Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
v10:
- Rebase
- Return NULL on CAP_DAC_READ_SEARCH
- Add __get xattr method to solve sepolicy logging issue
- Drop unnecassary sys_admin sepolicy checking for administrative
driver internal xattr functions.
v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation, drop rationalizations.
- pr_warn message adjusted to report consequences.
v5:
- beefed up the caveats in the Documentation
- Is dependent on
"overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
"overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off
v4:
- spelling and grammar errors in text
v3:
- Change name from caller_credentials / creator_credentials to the
boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.
^ permalink raw reply
* Re: [PATCH v1 1/2] mm/page_idle: Add support for per-pid page_idle using virtual indexing
From: Joel Fernandes @ 2019-07-24 19:33 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, vdavydov.dev, Brendan Gregg, kernel-team,
Alexey Dobriyan, Al Viro, carmenjackson, Christian Hansen,
Colin Ian King, dancol, David Howells, fmayer, joaodias,
Jonathan Corbet, Kees Cook, Kirill Tkhai, Konstantin Khlebnikov,
linux-doc, linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport,
minchan, minchan, namhyung, sspatil, surenb, Thomas Gleixner,
timmurray, tkjos, Vlastimil Babka, wvw
In-Reply-To: <20190722150639.27641c63b003dd04e187fd96@linux-foundation.org>
On Mon, Jul 22, 2019 at 03:06:39PM -0700, Andrew Morton wrote:
[snip]
> > + *end = *start + count * BITS_PER_BYTE;
> > + if (*end > max_frame)
> > + *end = max_frame;
> > + return 0;
> > +}
> > +
> >
> > ...
> >
> > +static void add_page_idle_list(struct page *page,
> > + unsigned long addr, struct mm_walk *walk)
> > +{
> > + struct page *page_get;
> > + struct page_node *pn;
> > + int bit;
> > + unsigned long frames;
> > + struct page_idle_proc_priv *priv = walk->private;
> > + u64 *chunk = (u64 *)priv->buffer;
> > +
> > + if (priv->write) {
> > + /* Find whether this page was asked to be marked */
> > + frames = (addr - priv->start_addr) >> PAGE_SHIFT;
> > + bit = frames % BITMAP_CHUNK_BITS;
> > + chunk = &chunk[frames / BITMAP_CHUNK_BITS];
> > + if (((*chunk >> bit) & 1) == 0)
> > + return;
> > + }
> > +
> > + page_get = page_idle_get_page(page);
> > + if (!page_get)
> > + return;
> > +
> > + pn = kmalloc(sizeof(*pn), GFP_ATOMIC);
>
> I'm not liking this GFP_ATOMIC. If I'm reading the code correctly,
> userspace can ask for an arbitrarily large number of GFP_ATOMIC
> allocations by doing a large read. This can potentially exhaust page
> reserves which things like networking Rx interrupts need and can make
> this whole feature less reliable.
For the revision, I will pre-allocate the page nodes in advance so it does
not need to do this. Diff on top of this patch is below. Let me know any
comments, thanks.
Btw, I also dropped the idle_page_list_lock by putting the idle_page_list
list_head on the stack instead of heap.
---8<-----------------------
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [PATCH] mm/page_idle: Avoid need for GFP_ATOMIC
GFP_ATOMIC can harm allocations does by other allocations that are in
need of reserves and the like. Pre-allocate the nodes list so that
spinlocked region can just use it.
Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
mm/page_idle.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/mm/page_idle.c b/mm/page_idle.c
index 874a60c41fef..b9c790721f16 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -266,6 +266,10 @@ struct page_idle_proc_priv {
unsigned long start_addr;
char *buffer;
int write;
+
+ /* Pre-allocate and provide nodes to add_page_idle_list() */
+ struct page_node *page_nodes;
+ int cur_page_node;
};
static void add_page_idle_list(struct page *page,
@@ -291,10 +295,7 @@ static void add_page_idle_list(struct page *page,
if (!page_get)
return;
- pn = kmalloc(sizeof(*pn), GFP_ATOMIC);
- if (!pn)
- return;
-
+ pn = &(priv->page_nodes[priv->cur_page_node++]);
pn->page = page_get;
pn->addr = addr;
list_add(&pn->list, &idle_page_list);
@@ -379,6 +380,15 @@ ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
priv.buffer = buffer;
priv.start_addr = start_addr;
priv.write = write;
+
+ priv.cur_page_node = 0;
+ priv.page_nodes = kzalloc(sizeof(struct page_node) * (end_frame - start_frame),
+ GFP_KERNEL);
+ if (!priv.page_nodes) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
walk.private = &priv;
walk.mm = mm;
@@ -425,6 +435,7 @@ ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
ret = copy_to_user(ubuff, buffer, count);
up_read(&mm->mmap_sem);
+ kfree(priv.page_nodes);
out:
kfree(buffer);
out_mmput:
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* Re: [PATCH v10 3/5] overlayfs: add __get xattr method
From: Amir Goldstein @ 2019-07-25 5:48 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <20190724195719.218307-4-salyzyn@android.com>
On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> Because of the overlayfs getxattr recursion, the incoming inode fails
> to update the selinux sid resulting in avc denials being reported
> against a target context of u:object_r:unlabeled:s0.
This description is too brief for me to understand the root problem.
What's wring with the overlayfs getxattr recursion w.r.t the selinux
security model?
Please give an example of your unprivileged mounter use case
to explain.
CC Vivek because I could really never understand all this.
>
> Solution is to add a _get xattr method that calls the __vfs_getxattr
> handler so that the context can be read in, rather than being denied
> with an -EACCES when vfs_getxattr handler is called.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> ---
> v10 - added to patch series
> ---
> fs/overlayfs/inode.c | 15 +++++++++++++++
> fs/overlayfs/overlayfs.h | 2 ++
> fs/overlayfs/super.c | 18 ++++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 7663aeb85fa3..d3b53849615c 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -362,6 +362,21 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> return err;
> }
>
> +int __ovl_xattr_get(struct dentry *dentry, struct inode *inode,
> + const char *name, void *value, size_t size)
> +{
> + ssize_t res;
> + const struct cred *old_cred;
> + struct dentry *realdentry =
> + ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
> +
> + old_cred = ovl_override_creds(dentry->d_sb);
> + res = __vfs_getxattr(realdentry, d_inode(realdentry), name, value,
> + size);
> + ovl_revert_creds(old_cred);
> + return res;
> +}
> +
> int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
> void *value, size_t size)
> {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 6934bcf030f0..73a02a263fbc 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -357,6 +357,8 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> const void *value, size_t size, int flags);
> int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
> void *value, size_t size);
> +int __ovl_xattr_get(struct dentry *dentry, struct inode *inode,
> + const char *name, void *value, size_t size);
> ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
> struct posix_acl *ovl_get_acl(struct inode *inode, int type);
> int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index b368e2e102fa..82e1130de206 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -859,6 +859,14 @@ ovl_posix_acl_xattr_get(const struct xattr_handler *handler,
> return ovl_xattr_get(dentry, inode, handler->name, buffer, size);
> }
>
> +static int __maybe_unused
> +__ovl_posix_acl_xattr_get(const struct xattr_handler *handler,
> + struct dentry *dentry, struct inode *inode,
> + const char *name, void *buffer, size_t size)
> +{
> + return __ovl_xattr_get(dentry, inode, handler->name, buffer, size);
> +}
> +
> static int __maybe_unused
> ovl_posix_acl_xattr_set(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> @@ -939,6 +947,13 @@ static int ovl_other_xattr_get(const struct xattr_handler *handler,
> return ovl_xattr_get(dentry, inode, name, buffer, size);
> }
>
> +static int __ovl_other_xattr_get(const struct xattr_handler *handler,
> + struct dentry *dentry, struct inode *inode,
> + const char *name, void *buffer, size_t size)
> +{
> + return __ovl_xattr_get(dentry, inode, name, buffer, size);
> +}
> +
> static int ovl_other_xattr_set(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> const char *name, const void *value,
> @@ -952,6 +967,7 @@ ovl_posix_acl_access_xattr_handler = {
> .name = XATTR_NAME_POSIX_ACL_ACCESS,
> .flags = ACL_TYPE_ACCESS,
> .get = ovl_posix_acl_xattr_get,
> + .__get = __ovl_posix_acl_xattr_get,
> .set = ovl_posix_acl_xattr_set,
> };
>
> @@ -960,6 +976,7 @@ ovl_posix_acl_default_xattr_handler = {
> .name = XATTR_NAME_POSIX_ACL_DEFAULT,
> .flags = ACL_TYPE_DEFAULT,
> .get = ovl_posix_acl_xattr_get,
> + .__get = __ovl_posix_acl_xattr_get,
> .set = ovl_posix_acl_xattr_set,
> };
>
> @@ -972,6 +989,7 @@ static const struct xattr_handler ovl_own_xattr_handler = {
> static const struct xattr_handler ovl_other_xattr_handler = {
> .prefix = "", /* catch all */
> .get = ovl_other_xattr_get,
> + .__get = __ovl_other_xattr_get,
> .set = ovl_other_xattr_set,
> };
>
Not very professional of me to comment on the proposed solution
without understanding the problem, but my nose says this cannot
be the right solution and if it is, then you better find a much better
name for the API then __get() and document it properly.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v10 5/5] overlayfs: override_creds=off option bypass creator_cred
From: Amir Goldstein @ 2019-07-25 6:14 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <20190724195719.218307-6-salyzyn@android.com>
On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> By default, all access to the upper, lower and work directories is the
> recorded mounter's MAC and DAC credentials. The incoming accesses are
> checked against the caller's credentials.
>
> If the principles of least privilege are applied, the mounter's
> credentials might not overlap the credentials of the caller's when
> accessing the overlayfs filesystem. For example, a file that a lower
> DAC privileged caller can execute, is MAC denied to the generally
> higher DAC privileged mounter, to prevent an attack vector.
>
> We add the option to turn off override_creds in the mount options; all
> subsequent operations after mount on the filesystem will be only the
> caller's credentials. The module boolean parameter and mount option
> override_creds is also added as a presence check for this "feature",
> existence of /sys/module/overlay/parameters/override_creds.
>
> It was not always this way. Circa 4.6 there was no recorded mounter's
> credentials, instead privileged access to upper or work directories
> were temporarily increased to perform the operations. The MAC
> (selinux) policies were caller's in all cases. override_creds=off
> partially returns us to this older access model minus the insecure
> temporary credential increases. This is to permit use in a system
> with non-overlapping security models for each executable including
> the agent that mounts the overlayfs filesystem. In Android
> this is the case since init, which performs the mount operations,
> has a minimal MAC set of privileges to reduce any attack surface,
> and services that use the content have a different set of MAC
> privileges (eg: read, for vendor labelled configuration, execute for
> vendor libraries and modules). The caveats are not a problem in
> the Android usage model, however they should be fixed for
> completeness and for general use in time.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> ---
> v10:
> - Rebase (and expand because of increased revert_cred usage)
>
> v9:
> - Add to the caveats
>
> v8:
> - drop pr_warn message after straw poll to remove it.
> - added a use case in the commit message
>
> v7:
> - change name of internal parameter to ovl_override_creds_def
> - report override_creds only if different than default
>
> v6:
> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
> - Do better with the documentation.
> - pr_warn message adjusted to report consequences.
>
> v5:
> - beefed up the caveats in the Documentation
> - Is dependent on
> "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
> "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
> - Added prwarn when override_creds=off
>
> v4:
> - spelling and grammar errors in text
>
> v3:
> - Change name from caller_credentials / creator_credentials to the
> boolean override_creds.
> - Changed from creator to mounter credentials.
> - Updated and fortified the documentation.
> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>
> v2:
> - Forward port changed attr to stat, resulting in a build error.
> - altered commit message.
>
> a
> ---
> Documentation/filesystems/overlayfs.txt | 23 +++++++++++++++++++++++
> fs/overlayfs/copy_up.c | 2 +-
> fs/overlayfs/dir.c | 11 ++++++-----
> fs/overlayfs/file.c | 20 ++++++++++----------
> fs/overlayfs/inode.c | 18 +++++++++---------
> fs/overlayfs/namei.c | 6 +++---
> fs/overlayfs/overlayfs.h | 1 +
> fs/overlayfs/ovl_entry.h | 1 +
> fs/overlayfs/readdir.c | 4 ++--
> fs/overlayfs/super.c | 22 +++++++++++++++++++++-
> fs/overlayfs/util.c | 12 ++++++++++--
> 11 files changed, 87 insertions(+), 33 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index 1da2f1668f08..d48125076602 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -102,6 +102,29 @@ Only the lists of names from directories are merged. Other content
> such as metadata and extended attributes are reported for the upper
> directory only. These attributes of the lower directory are hidden.
>
> +credentials
> +-----------
> +
> +By default, all access to the upper, lower and work directories is the
> +recorded mounter's MAC and DAC credentials. The incoming accesses are
> +checked against the caller's credentials.
> +
> +In the case where caller MAC or DAC credentials do not overlap, a
> +use case available in older versions of the driver, the
> +override_creds mount flag can be turned off and help when the use
> +pattern has caller with legitimate credentials where the mounter
> +does not. Several unintended side effects will occur though. The
> +caller without certain key capabilities or lower privilege will not
> +always be able to delete files or directories, create nodes, or
> +search some restricted directories. The ability to search and read
> +a directory entry is spotty as a result of the cache mechanism not
> +retesting the credentials because of the assumption, a privileged
> +caller can fill cache, then a lower privilege can read the directory
> +cache. The uneven security model where cache, upperdir and workdir
> +are opened at privilege, but accessed without creating a form of
> +privilege escalation, should only be used with strict understanding
> +of the side effects and of the security policies.
> +
> whiteouts and opaque directories
> --------------------------------
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index b801c6353100..1311ab4aea00 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -886,7 +886,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
> dput(parent);
> dput(next);
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 702aa63f6774..c4b061c3a6ef 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -563,7 +563,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> override_cred->fsgid = inode->i_gid;
> if (!attr->hardlink) {
> err = security_dentry_create_files_as(dentry,
> - attr->mode, &dentry->d_name, old_cred,
> + attr->mode, &dentry->d_name,
> + old_cred ? old_cred : current_cred(),
> override_cred);
> if (err) {
> put_cred(override_cred);
> @@ -579,7 +580,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> err = ovl_create_over_whiteout(dentry, inode, attr);
> }
> out_revert_creds:
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return err;
> }
>
> @@ -655,7 +656,7 @@ static int ovl_set_link_redirect(struct dentry *dentry)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> err = ovl_set_redirect(dentry, false);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> @@ -851,7 +852,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
> err = ovl_remove_upper(dentry, is_dir, &list);
> else
> err = ovl_remove_and_whiteout(dentry, &list);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (!err) {
> if (is_dir)
> clear_nlink(dentry->d_inode);
> @@ -1221,7 +1222,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
> out_unlock:
> unlock_rename(new_upperdir, old_upperdir);
> out_revert_creds:
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (update_nlink)
> ovl_nlink_end(new);
> out_drop_write:
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index e235a635d9ec..39a50fad9f7f 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -32,7 +32,7 @@ static struct file *ovl_open_realfile(const struct file *file,
> old_cred = ovl_override_creds(inode->i_sb);
> realfile = open_with_fake_path(&file->f_path, flags, realinode,
> current_cred());
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
> file, file, ovl_whatisit(inode, realinode), file->f_flags,
> @@ -176,7 +176,7 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
>
> old_cred = ovl_override_creds(inode->i_sb);
> ret = vfs_llseek(real.file, offset, whence);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> file->f_pos = real.file->f_pos;
> inode_unlock(inode);
> @@ -242,7 +242,7 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = vfs_iter_read(real.file, iter, &iocb->ki_pos,
> ovl_iocb_to_rwf(iocb));
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> ovl_file_accessed(file);
>
> @@ -278,7 +278,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> ret = vfs_iter_write(real.file, iter, &iocb->ki_pos,
> ovl_iocb_to_rwf(iocb));
> file_end_write(real.file);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> /* Update size */
> ovl_copyattr(ovl_inode_real(inode), inode);
> @@ -305,7 +305,7 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = vfs_fsync_range(real.file, start, end, datasync);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> }
>
> fdput(real);
> @@ -329,7 +329,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
>
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = call_mmap(vma->vm_file, vma);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> if (ret) {
> /* Drop reference count from new vm_file value */
> @@ -357,7 +357,7 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
>
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = vfs_fallocate(real.file, mode, offset, len);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> /* Update size */
> ovl_copyattr(ovl_inode_real(inode), inode);
> @@ -379,7 +379,7 @@ static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
>
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = vfs_fadvise(real.file, offset, len, advice);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> fdput(real);
>
> @@ -399,7 +399,7 @@ static long ovl_real_ioctl(struct file *file, unsigned int cmd,
>
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = vfs_ioctl(real.file, cmd, arg);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> fdput(real);
>
> @@ -589,7 +589,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> flags);
> break;
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> /* Update size */
> ovl_copyattr(ovl_inode_real(inode_out), inode_out);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index d3b53849615c..6c11c7af5157 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -61,7 +61,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> inode_lock(upperdentry->d_inode);
> old_cred = ovl_override_creds(dentry->d_sb);
> err = notify_change(upperdentry, attr, NULL);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (!err)
> ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
> inode_unlock(upperdentry->d_inode);
> @@ -257,7 +257,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> stat->nlink = dentry->d_inode->i_nlink;
>
> out:
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> @@ -291,7 +291,7 @@ int ovl_permission(struct inode *inode, int mask)
> mask |= MAY_READ;
> }
> err = inode_permission(realinode, mask);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> @@ -308,7 +308,7 @@ static const char *ovl_get_link(struct dentry *dentry,
>
> old_cred = ovl_override_creds(dentry->d_sb);
> p = vfs_get_link(ovl_dentry_real(dentry), done);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return p;
> }
>
> @@ -351,7 +351,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> WARN_ON(flags != XATTR_REPLACE);
> err = vfs_removexattr(realdentry, name);
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> /* copy c/mtime */
> ovl_copyattr(d_inode(realdentry), inode);
> @@ -387,7 +387,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
>
> old_cred = ovl_override_creds(dentry->d_sb);
> res = vfs_getxattr(realdentry, name, value, size);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return res;
> }
>
> @@ -411,7 +411,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> res = vfs_listxattr(realdentry, list, size);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (res <= 0 || size == 0)
> return res;
>
> @@ -446,7 +446,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
>
> old_cred = ovl_override_creds(inode->i_sb);
> acl = get_acl(realinode, type);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return acl;
> }
> @@ -484,7 +484,7 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> filemap_write_and_wait(realinode->i_mapping);
>
> err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index fb6c0cd7b65f..12627018b00a 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1079,7 +1079,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> goto out_free_oe;
> }
>
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (origin_path) {
> dput(origin_path->dentry);
> kfree(origin_path);
> @@ -1106,7 +1106,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> kfree(upperredirect);
> out:
> kfree(d.redirect);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> return ERR_PTR(err);
> }
>
> @@ -1160,7 +1160,7 @@ bool ovl_lower_positive(struct dentry *dentry)
> dput(this);
> }
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return positive;
> }
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 82574684a9b6..cdbdb533d3bd 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -205,6 +205,7 @@ int ovl_want_write(struct dentry *dentry);
> void ovl_drop_write(struct dentry *dentry);
> struct dentry *ovl_workdir(struct dentry *dentry);
> const struct cred *ovl_override_creds(struct super_block *sb);
> +void ovl_revert_creds(const struct cred *oldcred);
> ssize_t ovl_vfs_getxattr(struct dentry *dentry, const char *name, void *buf,
> size_t size);
> struct super_block *ovl_same_sb(struct super_block *sb);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 28a2d12a1029..2637c5aadf7f 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -17,6 +17,7 @@ struct ovl_config {
> bool nfs_export;
> int xino;
> bool metacopy;
> + bool override_creds;
> };
>
> struct ovl_sb {
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 47a91c9733a5..f31ef39e5afa 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -286,7 +286,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
> }
> inode_unlock(dir->d_inode);
> }
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
>
> return err;
> }
> @@ -918,7 +918,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
>
> old_cred = ovl_override_creds(dentry->d_sb);
> err = ovl_dir_read_merged(dentry, list, &root);
> - revert_creds(old_cred);
> + ovl_revert_creds(old_cred);
> if (err)
> return err;
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 82e1130de206..c2ddce5d488c 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -53,6 +53,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
> MODULE_PARM_DESC(xino_auto,
> "Auto enable xino feature");
>
> +static bool __read_mostly ovl_override_creds_def = true;
> +module_param_named(override_creds, ovl_override_creds_def, bool, 0644);
> +MODULE_PARM_DESC(ovl_override_creds_def,
> + "Use mounter's credentials for accesses");
> +
> static void ovl_entry_stack_free(struct ovl_entry *oe)
> {
> unsigned int i;
> @@ -362,6 +367,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> if (ofs->config.metacopy != ovl_metacopy_def)
> seq_printf(m, ",metacopy=%s",
> ofs->config.metacopy ? "on" : "off");
> + if (ofs->config.override_creds != ovl_override_creds_def)
> + seq_show_option(m, "override_creds",
> + ofs->config.override_creds ? "on" : "off");
> return 0;
> }
>
> @@ -402,6 +410,8 @@ enum {
> OPT_XINO_AUTO,
> OPT_METACOPY_ON,
> OPT_METACOPY_OFF,
> + OPT_OVERRIDE_CREDS_ON,
> + OPT_OVERRIDE_CREDS_OFF,
> OPT_ERR,
> };
>
> @@ -420,6 +430,8 @@ static const match_table_t ovl_tokens = {
> {OPT_XINO_AUTO, "xino=auto"},
> {OPT_METACOPY_ON, "metacopy=on"},
> {OPT_METACOPY_OFF, "metacopy=off"},
> + {OPT_OVERRIDE_CREDS_ON, "override_creds=on"},
> + {OPT_OVERRIDE_CREDS_OFF, "override_creds=off"},
> {OPT_ERR, NULL}
> };
>
> @@ -478,6 +490,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
> if (!config->redirect_mode)
> return -ENOMEM;
> + config->override_creds = ovl_override_creds_def;
>
> while ((p = ovl_next_opt(&opt)) != NULL) {
> int token;
> @@ -558,6 +571,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->metacopy = false;
> break;
>
> + case OPT_OVERRIDE_CREDS_ON:
> + config->override_creds = true;
> + break;
> +
> + case OPT_OVERRIDE_CREDS_OFF:
> + config->override_creds = false;
> + break;
> +
> default:
> pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
> return -EINVAL;
> @@ -1690,7 +1711,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> ovl_dentry_lower(root_dentry), NULL);
>
> sb->s_root = root_dentry;
> -
> return 0;
>
> out_free_oe:
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 672459c3cff7..320aad599bcd 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -37,9 +37,17 @@ const struct cred *ovl_override_creds(struct super_block *sb)
> {
> struct ovl_fs *ofs = sb->s_fs_info;
>
> + if (!ofs->config.override_creds)
> + return NULL;
> return override_creds(ofs->creator_cred);
> }
>
> +void ovl_revert_creds(const struct cred *old_cred)
> +{
> + if (old_cred)
> + revert_creds(old_cred);
> +}
> +
Mark,
Not sure if you have seen my "shutdown" patches:
https://lore.kernel.org/linux-fsdevel/20190715133839.9878-4-amir73il@gmail.com/
I am fine with this patch, but would like to request that you add @sb arg
to the ovl_revert_creds() helper, so it is more useful for other things in the
future that scope the underlying layers access (like shutdown).
Thanks,
Amir.
^ permalink raw reply
* RE: [PATCH v5] Documentation/checkpatch: Prefer strscpy/strscpy_pad over strcpy/strlcpy/strncpy
From: Gote, Nitin R @ 2019-07-25 7:26 UTC (permalink / raw)
To: Joe Perches, Kees Cook
Cc: corbet@lwn.net, akpm@linux-foundation.org, apw@canonical.com,
linux-doc@vger.kernel.org, kernel-hardening@lists.openwall.com
In-Reply-To: <0d69778626901a841108ae024b8a105da679d9af.camel@perches.com>
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Wednesday, July 24, 2019 11:59 PM
> To: Gote, Nitin R <nitin.r.gote@intel.com>; Kees Cook
> <keescook@chromium.org>
> Cc: corbet@lwn.net; akpm@linux-foundation.org; apw@canonical.com;
> linux-doc@vger.kernel.org; kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH v5] Documentation/checkpatch: Prefer
> strscpy/strscpy_pad over strcpy/strlcpy/strncpy
>
> On Wed, 2019-07-24 at 18:17 +0000, Gote, Nitin R wrote:
> > Hi,
>
> Hi again.
>
> []
> > > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
>
> Please remember there does not exist a single actual use of strscpy_pad in
> the kernel sources and no apparent real need for it. I don't find one anyway.
>
Thanks for clarification. I will remove strscpy_pad() from patch.
> > Could you please give your opinion on below comment.
> >
> > > But, if the destination buffer needs extra NUL-padding for remaining
> > > size of destination, then safe replacement is strscpy_pad(). Right?
> > > If yes, then what is your opinion on below change :
> > >
> > > "strncpy" => "strscpy, strcpy_pad - for non-NUL-terminated
> > > uses,
> > > strncpy() dst should be __nonstring",
> > >
> > If you agree on this, then I will include this change in next patch version.
>
> Two things:
>
> The kernel-doc documentation uses dest not dst.
Noted. I will correct this.
> I think stracpy should be preferred over strscpy.
>
Agreed.
I will use stracpy() instead of strscpy().
Thanks,
Nitin
^ permalink raw reply
* Re: [PATCH v1 1/2] mm/page_idle: Add support for per-pid page_idle using virtual indexing
From: Konstantin Khlebnikov @ 2019-07-25 8:15 UTC (permalink / raw)
To: Joel Fernandes, Minchan Kim
Cc: linux-kernel, vdavydov.dev, Brendan Gregg, kernel-team,
Alexey Dobriyan, Al Viro, Andrew Morton, carmenjackson,
Christian Hansen, Colin Ian King, dancol, David Howells, fmayer,
joaodias, Jonathan Corbet, Kees Cook, Kirill Tkhai, linux-doc,
linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport, namhyung,
sspatil
In-Reply-To: <20190724141052.GB9945@google.com>
On 24.07.2019 17:10, Joel Fernandes wrote:> On Wed, Jul 24, 2019 at 01:28:42PM +0900, Minchan Kim wrote:
>> On Tue, Jul 23, 2019 at 10:20:49AM -0400, Joel Fernandes wrote:
>>> On Tue, Jul 23, 2019 at 03:13:58PM +0900, Minchan Kim wrote:
>>>> Hi Joel,
>>>>
>>>> On Mon, Jul 22, 2019 at 05:32:04PM -0400, Joel Fernandes (Google) wrote:
>>>>> The page_idle tracking feature currently requires looking up the pagemap
>>>>> for a process followed by interacting with /sys/kernel/mm/page_idle.
>>>>> This is quite cumbersome and can be error-prone too. If between
>>>>
>>>> cumbersome: That's the fair tradeoff between idle page tracking and
>>>> clear_refs because idle page tracking could check even though the page
>>>> is not mapped.
>>>
>>> It is fair tradeoff, but could be made simpler. The userspace code got
>>> reduced by a good amount as well.
>>>
>>>> error-prone: What's the error?
>>>
>>> We see in normal Android usage, that some of the times pages appear not to be
>>> idle even when they really are idle. Reproducing this is a bit unpredictable
>>> and happens at random occasions. With this new interface, we are seeing this
>>> happen much much lesser.
>>
>> I don't know how you did test. Maybe that could be contributed by
>> swapping out or shared pages touched by other processes or some kernel
>> behavior not to keep access bit of their operation.
>
> It could be something along these lines is my thinking as well. So we know
> its already has issues due to what you mentioned, I am not sure what else
> needs investigation?
>
>> Please investigate more what's the root cause. That would be important
>> point to justify for the patch motivation.
>
> The motivation is security. I am dropping the 'accuracy' factor I mentioned
> from the patch description since it created a lot of confusion.
If you are tracking idle working set for one process you could use degrading
'accuracy' for good - just don't walk page rmap and play only with access
bits in one process. Foreign access could be detected with arbitrary delay,
but this does not important if main goal is heap profiling.
>
>>>>> More over looking up PFN from pagemap in Android devices is not
>>>>> supported by unprivileged process and requires SYS_ADMIN and gives 0 for
>>>>> the PFN.
>>>>>
>>>>> This patch adds support to directly interact with page_idle tracking at
>>>>> the PID level by introducing a /proc/<pid>/page_idle file. This
>>>>> eliminates the need for userspace to calculate the mapping of the page.
>>>>> It follows the exact same semantics as the global
>>>>> /sys/kernel/mm/page_idle, however it is easier to use for some usecases
>>>>> where looking up PFN is not needed and also does not require SYS_ADMIN.
>>>>
>>>> Ah, so the primary goal is to provide convinience interface and it would
>>>> help accurary, too. IOW, accuracy is not your main goal?
>>>
>>> There are a couple of primary goals: Security, conveience and also solving
>>> the accuracy/reliability problem we are seeing. Do keep in mind looking up
>>> PFN has security implications. The PFN field in pagemap is zeroed if the user
>>> does not have CAP_SYS_ADMIN.
>>
>> Myaybe you don't need PFN. is it?
>
> With the traditional idle tracking, PFN is needed which has the mentioned
> security issues. This patch solves it. And the interface is identical and
> familiar to the existing page_idle bitmap interface.
>
>>>>> In Android, we are using this for the heap profiler (heapprofd) which
>>>>> profiles and pin points code paths which allocates and leaves memory
>>>>> idle for long periods of time.
>>>>
>>>> So the goal is to detect idle pages with idle memory tracking?
>>>
>>> Isn't that what idle memory tracking does?
>>
>> To me, it's rather misleading. Please read motivation section in document.
>> The feature would be good to detect workingset pages, not idle pages
>> because workingset pages are never freed, swapped out and even we could
>> count on newly allocated pages.
>>
>> Motivation
>> ==========
>>
>> The idle page tracking feature allows to track which memory pages are being
>> accessed by a workload and which are idle. This information can be useful for
>> estimating the workload's working set size, which, in turn, can be taken into
>> account when configuring the workload parameters, setting memory cgroup limits,
>> or deciding where to place the workload within a compute cluster.
>
> As we discussed by chat, we could collect additional metadata to check if
> pages were swapped or freed ever since the time we marked them as idle.
> However this can be incremental improvement.
>
>>>> It couldn't work well because such idle pages could finally swap out and
>>>> lose every flags of the page descriptor which is working mechanism of
>>>> idle page tracking. It should have named "workingset page tracking",
>>>> not "idle page tracking".
>>>
>>> The heap profiler that uses page-idle tracking is not to measure working set,
>>> but to look for pages that are idle for long periods of time.
>>
>> It's important part. Please include it in the description so that people
>> understands what's the usecase. As I said above, if it aims for finding
>> idle pages durting the period, current idle page tracking feature is not
>> good ironically.
>
> Ok, I will mention.
>
>>> Thanks for bringing up the swapping corner case.. Perhaps we can improve
>>> the heap profiler to detect this by looking at bits 0-4 in pagemap. While it
>>
>> Yeb, that could work but it could add overhead again what you want to remove?
>> Even, userspace should keep metadata to identify that page was already swapped
>> in last period or newly swapped in new period.
>
> Yep.
Between samples page could be read from swap and swapped out back multiple times.
For tracking this swap ptes could be marked with idle bit too.
I believe it's not so hard to find free bit for this.
Refault\swapout will automatically clear this bit in pte even if
page goes nowhere stays if swap-cache.
^ permalink raw reply
* Re: [PATCH v15 06/13] irqchip: Add irq-ingenic-tcu driver
From: Marc Zyngier @ 2019-07-25 8:21 UTC (permalink / raw)
To: Paul Cercueil, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Lee Jones, Arnd Bergmann, Daniel Lezcano,
Thomas Gleixner, Michael Turquette, Stephen Boyd, Jason Cooper,
Rob Herring, Mark Rutland
Cc: devicetree, linux-kernel, linux-doc, linux-mips, linux-clk, od,
Mathieu Malaterre, Artur Rojek
In-Reply-To: <20190724171615.20774-7-paul@crapouillou.net>
On 24/07/2019 18:16, Paul Cercueil wrote:
> This driver handles the interrupt controller built in the Timer/Counter
> Unit (TCU) of the JZ47xx SoCs from Ingenic.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Marc Zyngier <maz@kernel.org>
Given the various dependencies, I assume the series will get routed via
the MIPS tree.
Thanks,
M.
--
Jazz is not dead, it just smells funny...
^ permalink raw reply
* Re: [PATCH] hung_task: Allow printing warnings every check interval
From: Tetsuo Handa @ 2019-07-25 10:38 UTC (permalink / raw)
To: Dmitry Safonov, linux-kernel
Cc: Dmitry Safonov, Andrew Morton, Dmitry Vyukov, Ingo Molnar,
Jonathan Corbet, Thomas Gleixner, Peter Zijlstra (Intel),
Vasiliy Khoruzhick, linux-doc, linux-fsdevel
In-Reply-To: <20190724170249.9644-1-dima@arista.com>
On 2019/07/25 2:02, Dmitry Safonov wrote:
> Hung task detector has one timeout and has two associated actions on it:
> - issuing warnings with names and stacks of blocked tasks
> - panic()
>
> We want switches to panic (and reboot) if there's a task
> in uninterruptible sleep for some minutes - at that moment something
> ugly has happened and the box needs a reboot.
> But we also want to detect conditions that are "out of range"
> or approaching the point of failure. Under such conditions we want
> to issue an "early warning" of an impending failure, minutes before
> the switch is going to panic.
Can't we do it by extending sysctl_hung_task_panic to accept values larger
than 1, and decrease by one when at least one thread was reported by each
check_hung_uninterruptible_tasks() check, and call panic() when
sysctl_hung_task_panic reached to 0 (or maybe 1 is simpler) ?
Hmm, might have the same problem regarding how/when to reset the counter.
If some userspace process can reset the counter, such process can trigger
SysRq-c when some period expired...
> It seems rather easy to add printing tasks and their stacks for
> notification and debugging purposes into hung task detector without
> complicating the code or major cost (prints are with KERN_INFO loglevel
> and so don't go on console, only into dmesg log).
Well, I don't think so. Might be noisy for systems without "quiet" kernel
command line option, and we can't pass KERN_DEBUG to e.g. sched_show_task()...
^ permalink raw reply
* Re: [PATCH v10 4/5] overlayfs: internal getxattr operations without sepolicy checking
From: Amir Goldstein @ 2019-07-25 11:00 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <20190724195719.218307-5-salyzyn@android.com>
On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> Check impure, opaque, origin & meta xattr with no sepolicy audit
> (using __vfs_getxattr) since these operations are internal to
> overlayfs operations and do not disclose any data. This became
> an issue for credential override off since sys_admin would have
> been required by the caller; whereas would have been inherently
> present for the creator since it performed the mount.
>
> This is a change in operations since we do not check in the new
> ovl_vfs_getxattr function if the credential override is off or
> not. Reasoning is that the sepolicy check is unnecessary overhead,
> especially since the check can be expensive.
I don't know that this reasoning suffice to skip the sepolicy checks
for overlayfs private xattrs.
Can't sepolicy be defined to allow get access to trusted.overlay.*?
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> ---
> v10 - added to patch series
> ---
> fs/overlayfs/namei.c | 12 +++++++-----
> fs/overlayfs/overlayfs.h | 2 ++
> fs/overlayfs/util.c | 24 +++++++++++++++---------
> 3 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 9702f0d5309d..fb6c0cd7b65f 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -106,10 +106,11 @@ int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
>
> static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> {
> - int res, err;
> + ssize_t res;
> + int err;
> struct ovl_fh *fh = NULL;
>
> - res = vfs_getxattr(dentry, name, NULL, 0);
> + res = ovl_vfs_getxattr(dentry, name, NULL, 0);
> if (res < 0) {
> if (res == -ENODATA || res == -EOPNOTSUPP)
> return NULL;
> @@ -123,7 +124,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> if (!fh)
> return ERR_PTR(-ENOMEM);
>
> - res = vfs_getxattr(dentry, name, fh, res);
> + res = ovl_vfs_getxattr(dentry, name, fh, res);
> if (res < 0)
> goto fail;
>
> @@ -141,10 +142,11 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> return NULL;
>
> fail:
> - pr_warn_ratelimited("overlayfs: failed to get origin (%i)\n", res);
> + pr_warn_ratelimited("overlayfs: failed to get origin (%zi)\n", res);
> goto out;
> invalid:
> - pr_warn_ratelimited("overlayfs: invalid origin (%*phN)\n", res, fh);
> + pr_warn_ratelimited("overlayfs: invalid origin (%*phN)\n",
> + (int)res, fh);
> goto out;
> }
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 73a02a263fbc..82574684a9b6 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -205,6 +205,8 @@ int ovl_want_write(struct dentry *dentry);
> void ovl_drop_write(struct dentry *dentry);
> struct dentry *ovl_workdir(struct dentry *dentry);
> const struct cred *ovl_override_creds(struct super_block *sb);
> +ssize_t ovl_vfs_getxattr(struct dentry *dentry, const char *name, void *buf,
> + size_t size);
> struct super_block *ovl_same_sb(struct super_block *sb);
> int ovl_can_decode_fh(struct super_block *sb);
> struct dentry *ovl_indexdir(struct super_block *sb);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f5678a3f8350..672459c3cff7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -40,6 +40,12 @@ const struct cred *ovl_override_creds(struct super_block *sb)
> return override_creds(ofs->creator_cred);
> }
>
> +ssize_t ovl_vfs_getxattr(struct dentry *dentry, const char *name, void *buf,
> + size_t size)
> +{
> + return __vfs_getxattr(dentry, d_inode(dentry), name, buf, size);
> +}
> +
When introducing a new ovl_ => vfs_ wrapper, please follow the
ovl_do_XXX helpers
convention in overlayfs.h.
Note that those wrappers do not generally bypass security checks and
you have not
convinced me yet that skipping security checks on the overlayfs
private xattr get
is the right thing to do.
Thanks,
Amir.
^ permalink raw reply
* [PATCH v6] Documentation/checkpatch: Prefer stracpy over strcpy/strlcpy/strncpy.
From: NitinGote @ 2019-07-25 11:22 UTC (permalink / raw)
To: joe, keescook; +Cc: corbet, akpm, apw, linux-doc, kernel-hardening, Nitin Gote
From: Nitin Gote <nitin.r.gote@intel.com>
Added check in checkpatch.pl to deprecate strcpy(), strlcpy() and
strncpy() in favor of stracpy().
Updated Documentation/process/deprecated.rst for stracpy().
Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
---
Change log:
v5->v6
- Used stracpy() instead of strscpy().
v4->v5
- Change the subject line as per review comment.
- v5 is Reviewed-by: Kees Cook <keescook@chromium.org>
v3->v4
- Removed "c:func:" from deprecated.rst as per review comment.
v2->v3
- Avoided use of $check in implementation.
- Incorporated trivial comments.
v1->v2
- For string related apis, created different %deprecated_string_api
and these will get emitted at CHECK Level using command line option
-f/--file to avoid bad patched from novice script users.
Documentation/process/deprecated.rst | 10 +++++-----
scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 49e0f64a3427..709662c71a1a 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -84,7 +84,7 @@ buffer. This could result in linear overflows beyond the
end of the buffer, leading to all kinds of misbehaviors. While
`CONFIG_FORTIFY_SOURCE=y` and various compiler flags help reduce the
risk of using this function, there is no good reason to add new uses of
-this function. The safe replacement is :c:func:`strscpy`.
+this function. The safe replacement is stracpy().
strncpy() on NUL-terminated strings
-----------------------------------
@@ -93,9 +93,9 @@ will be NUL terminated. This can lead to various linear read overflows
and other misbehavior due to the missing termination. It also NUL-pads the
destination buffer if the source contents are shorter than the destination
buffer size, which may be a needless performance penalty for callers using
-only NUL-terminated strings. The safe replacement is :c:func:`strscpy`.
-(Users of :c:func:`strscpy` still needing NUL-padding will need an
-explicit :c:func:`memset` added.)
+only NUL-terminated strings. In this case, the safe replacement is
+stracpy(). If, however, the destination buffer still needs NUL-padding,
+the safe replacement is stracpy_pad().
If a caller is using non-NUL-terminated strings, :c:func:`strncpy()` can
still be used, but destinations should be marked with the `__nonstring
@@ -107,7 +107,7 @@ strlcpy()
:c:func:`strlcpy` reads the entire source buffer first, possibly exceeding
the given limit of bytes to copy. This is inefficient and can lead to
linear read overflows if a source string is not NUL-terminated. The
-safe replacement is :c:func:`strscpy`.
+safe replacement is stracpy().
Variable Length Arrays (VLAs)
-----------------------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 342c7c781ba5..dddf5adf1aac 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) {
}
$deprecated_apis_search = "(?:${deprecated_apis_search})";
+our %deprecated_string_apis = (
+ "strcpy" => "stracpy",
+ "strlcpy" => "stracpy",
+ "strncpy" => "stracpy - for non-NUL-terminated uses, strncpy dest should be __nonstring",
+);
+
+#Create a search pattern for all these strings apis to speed up a loop below
+our $deprecated_string_apis_search = "";
+foreach my $entry (keys %deprecated_string_apis) {
+ $deprecated_string_apis_search .= '|' if ($deprecated_string_apis_search ne "");
+ $deprecated_string_apis_search .= $entry;
+}
+$deprecated_string_apis_search = "(?:${deprecated_string_apis_search})";
+
our $mode_perms_world_writable = qr{
S_IWUGO |
S_IWOTH |
@@ -6446,6 +6460,16 @@ sub process {
"Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
}
+# check for string deprecated apis
+ if ($line =~ /\b($deprecated_string_apis_search)\b\s*\(/) {
+ my $deprecated_string_api = $1;
+ my $new_api = $deprecated_string_apis{$deprecated_string_api};
+ my $msg_level = \&WARN;
+ $msg_level = \&CHK if ($file);
+ &{$msg_level}("DEPRECATED_API",
+ "Deprecated use of '$deprecated_string_api', prefer '$new_api' instead\n" . $herecurr);
+ }
+
# check for various structs that are normally const (ops, kgdb, device_tree)
# and avoid what seem like struct definitions 'struct foo {'
if ($line !~ /\bconst\b/ &&
--
2.17.1
^ permalink raw reply related
* [PATCH] Documentation/admin-guide: Embargoed hardware security issues
From: Greg Kroah-Hartman @ 2019-07-25 13:01 UTC (permalink / raw)
To: linux-kernel, Jonathan Corbet
Cc: security, linux-doc, Thomas Gleixner, Jiri Kosina,
Mauro Carvalho Chehab
From: Thomas Gleixner <tglx@linutronix.de>
To address the requirements of embargoed hardware issues, like Meltdown,
Spectre, L1TF, etc. it is necessary to define and document a process for
handling embargoed hardware security issues.
Following the discussion at the maintainer summit 2018 in Edinburgh
(https://lwn.net/Articles/769417/) the volunteered people have worked
out a process and a Memorandum of Understanding. The latter addresses
the fact that the Linux kernel community cannot sign NDAs for various
reasons.
The initial contact point for hardware security issues is different from
the regular kernel security contact to provide a known and neutral
interface for hardware vendors and researchers. The initial primary
contact team is proposed to be staffed by Linux Foundation Fellows, who
are not associated to a vendor or a distribution and are well connected
in the industry as a whole.
The process is designed with the experience of the past incidents in
mind and tries to address the remaining gaps, so future (hopefully rare)
incidents can be handled more efficiently. It won’t remove the fact,
that most of this has to be done behind closed doors, but it is set up
to avoid big bureaucratic hurdles for individual developers.
The process is solely for handling hardware security issues and cannot
be used for regular kernel (software only) security bugs.
The process can help with hardware companies who, and I quote, "[my
manager] doesn't want to bet his job on the list keeping things secret."
This despite numerous leaks directly from that company over the years,
and none ever so far from the kernel security team. Cognitive
dissidence seems to be a requirement to be a "successful" manager.
To accelerate the adoption of this process, we introduce the concept of
ambassadors in participating companies. The ambassadors are there to
guide people to comply with the process, but are not automatically
involved in the disclosure of a particular incident.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/admin-guide/embargoed-hardware-issues.rst | 281 ++++++++++++++++
Documentation/admin-guide/index.rst | 1
2 files changed, 282 insertions(+)
Note, this document has gone through numerous reviews by a number of
kernel developers, developers at some of the Linux distros, as well as
all of the lawyers from almost all open source-related companies. It's
been sitting on my local drive with no comments for a few months now,
and it's about time to get this out and merged properly.
If anyone has any final comments, please let me know.
If anyone from any company listed below wishes to add their name to the
document, please send a follow-on patch and I will be glad to add it to
the series. I had a number of "I'll sign up" type comments from
different people, but I want something with a "s-o-b" to keep people on
the hook for this, so I did not add their name to the file without that.
thanks,
greg k-h
--- /dev/null
+++ b/Documentation/admin-guide/embargoed-hardware-issues.rst
@@ -0,0 +1,281 @@
+.. _embargoedhardwareissues:
+
+Embargoed hardware issues
+=========================
+
+Scope
+-----
+
+Hardware issues which result in security problems are a different category
+of security bugs than pure software bugs which only affect the Linux
+kernel.
+
+Hardware issues like Meltdown, Spectre, L1TF etc. must be treated
+differently because they usually affect all Operating Systems (“OS“) and
+therefore need coordination across different OS vendors, distributions,
+hardware vendors and other parties. For some of the issues, software
+mitigations can depend on microcode or firmware updates, which need further
+coordination.
+
+.. _Contact:
+
+Contact
+-------
+
+The Linux kernel hardware security team is separate from the regular Linux
+kernel security team.
+
+The team is only handling the coordination of embargoed hardware security
+issues. Reports of pure software security bugs in the Linux kernel are not
+handled by this team and the reporter will be guided to contact the regular
+Linux kernel security team (:ref:`Documentation/admin-guide/
+<securitybugs>`) instead.
+
+The team can be contacted by email at <hardware-security@kernel.org>. This
+is a private list of security officers who will help you to coordinate an
+issue according to our documented process.
+
+The list is encrypted and email to the list can be sent by either PGP or
+S/MIME encrypted and must be signed with the reporter's PGP key or S/MIME
+certificate. The list's PGP key and S/MIME certificate are available from
+https://www.kernel.org/....
+
+While hardware security issues are often handled by the affected hardware
+vendor, we welcome contact from researchers or individuals who identified a
+potential hardware flaw.
+
+Hardware security officers
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The current team of hardware security officers:
+
+ - Linus Torvalds (Linux Foundation Fellow)
+ - Greg Kroah-Hartman (Linux Foundation Fellow)
+ - Thomas Gleixner (Linux Foundation Fellow)
+
+Operation of mailing-lists
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The encrypted mailing-lists which are used in our process are hosted on
+Linux Foundation's IT infrastructure. By providing this service Linux
+Foundation's director of IT Infrastructure security technically has the
+ability to access the embargoed information, but is obliged to
+confidentiality by his employment contract. Linux Foundation's director of
+IT Infrastructure security is also responsible for the kernel.org
+infrastructure.
+
+The Linux Foundation's current director of IT Infrastructure security is
+Konstantin Ryabitsev.
+
+
+Non-disclosure agreements
+-------------------------
+
+The Linux kernel hardware security team is not a formal body and therefore
+unable to enter into any non-disclosure agreements. The kernel community
+is aware of the sensitive nature of such issues and offers a Memorandum of
+Understanding instead.
+
+
+Memorandum of Understanding
+---------------------------
+
+The Linux kernel community has a deep understanding of the requirement to
+keep hardware security issues under embargo for coordination between
+different OS vendors, distributors, hardware vendors and other parties.
+
+The Linux kernel community has successfully handled hardware security
+issues in the past and has the necessary mechanisms in place to allow
+community compliant development under embargo restrictions.
+
+The Linux kernel community has a dedicated hardware security team for
+initial contact, which oversees the process of handling such issues under
+embargo rules.
+
+The hardware security team identifies the developers (domain experts) which
+form the initial response team for a particular issue. The initial response
+team can bring in further developers (domain experts) to address the issue
+in the best technical way.
+
+All involved developers pledge to adhere to the embargo rules and to keep
+the received information confidential. Violation of the pledge will lead to
+immediate exclusion from the current issue and removal from all related
+mailing-lists. In addition, the hardware security team will also exclude
+the offender from future issues. The impact of this consequence is a highly
+effective deterrent in our community. In case a violation happens the
+hardware security team will inform the involved parties immediately. If you
+or anyone becomes aware of a potential violation, please report it
+immediately to the Hardware security officers.
+
+
+Process
+^^^^^^^
+
+Due to the globally distributed nature of Linux kernel development, face to
+face meetings are almost impossible to address hardware security issues.
+Phone conferences are hard to coordinate due to time zones and other
+factors and should be only used when absolutely necessary. Encrypted email
+has been proven to be the most effective and secure communication method
+for these types of issues.
+
+Start of Disclosure
+"""""""""""""""""""
+
+Disclosure starts by contacting the Linux kernel hardware security team by
+email. This initial contact should contain a description of the problem and
+a list of any known affected hardware. If your organization builds or
+distributes the affected hardware, we encourage you to also consider what
+other hardware could be affected.
+
+The hardware security team will provide a per incident specific encrypted
+mailing-list which will be used for initial discussion with the reporter,
+further disclosure and coordination.
+
+The hardware security team will provide the disclosing party a list of
+developers (domain experts) who should be informed initially about the
+issue after confirming with the developers that they will adhere to this
+Memorandum of Understanding and the documented process. These developers
+form the initial response team and will be responsible for handling the
+issue after initial contact. The hardware security team is supporting the
+response team, but is not necessarily involved in the mitigation
+development process.
+
+While individual developers might be covered by a non-disclosure agreement
+via their employer, they cannot enter individual non-disclosure agreements
+in their role as Linux kernel developers. They will, however, adhere to
+this documented process and the Memorandum of Understanding.
+
+
+Disclosure
+""""""""""
+
+The disclosing party provides detailed information to the initial response
+team via the specific encrypted mailing-list.
+
+From our experience the technical documentation of these issues is usually
+a sufficient starting point and further technical clarification is best
+done via email.
+
+Mitigation development
+""""""""""""""""""""""
+
+The initial response team sets up an encrypted mailing-list or repurposes
+an existing one if appropriate. The disclosing party should provide a list
+of contacts for all other parties who have already been, or should be
+informed about the issue. The response team contacts these parties so they
+can name experts who should be subscribed to the mailing-list.
+
+Using a mailing-list is close to the normal Linux development process and
+has been successfully used in developing mitigations for various hardware
+security issues in the past.
+
+The mailing-list operates in the same way as normal Linux development.
+Patches are posted, discussed and reviewed and if agreed on applied to a
+non-public git repository which is only accessible to the participating
+developers via a secure connection. The repository contains the main
+development branch against the mainline kernel and backport branches for
+stable kernel versions as necessary.
+
+The initial response team will identify further experts from the Linux
+kernel developer community as needed and inform the disclosing party about
+their participation. Bringing in experts can happen at any time of the
+development process and often needs to be handled in a timely manner.
+
+Coordinated release
+"""""""""""""""""""
+
+The involved parties will negotiate the date and time where the embargo
+ends. At that point the prepared mitigations are integrated into the
+relevant kernel trees and published.
+
+While we understand that hardware security issues need coordinated embargo
+time, the embargo time should be constrained to the minimum time which is
+required for all involved parties to develop, test and prepare the
+mitigations. Extending embargo time artificially to meet conference talk
+dates or other non-technical reasons is creating more work and burden for
+the involved developers and response teams as the patches need to be kept
+up to date in order to follow the ongoing upstream kernel development,
+which might create conflicting changes.
+
+CVE assignment
+""""""""""""""
+
+Neither the hardware security team nor the initial response team assign
+CVEs, nor are CVEs required for the development process. If CVEs are
+provided by the disclosing party they can be used for documentation
+purposes.
+
+Process ambassadors
+-------------------
+
+For assistance with this process we have established ambassadors in various
+organizations, who can answer questions about or provide guidance on the
+reporting process and further handling. Ambassadors are not involved in the
+disclosure of a particular issue, unless requested by a response team or by
+an involved disclosed party. The current ambassadors list:
+
+ ============== ========================================================
+ ARM
+ AMD
+ IBM
+ Intel
+ Qualcomm
+
+ Microsoft
+ VMware
+ XEN
+
+ Canonical
+ Debian
+ Oracle
+ Redhat
+ Suse Jiri Kosina <jkosina@suse.com>
+
+ Amazon
+ Google
+ ============== ========================================================
+
+If you want your organization to be added to the ambassadors list, please
+contact the hardware security team. The nominated ambassador has to
+understand and support our process fully and is ideally well connected in
+the Linux kernel community.
+
+Encrypted mailing-lists
+-----------------------
+
+We use encrypted mailing-lists for communication. The operating principle
+of these lists is that email sent to the list is encrypted either with the
+list's PGP key or with the list's S/MIME certificate. The mailing-list
+software decrypts the email and re-encrypts it individually for each
+subscriber with the subscriber's PGP key or S/MIME certificate. Details
+about the mailing-list software and the setup which is used to ensure the
+security of the lists and protection of the data can be found here:
+https://www.kernel.org/....
+
+List keys
+^^^^^^^^^
+
+For initial contact see :ref:`Contact`. For incident specific mailing-lists
+the key and S/MIME certificate are conveyed to the subscribers by email
+sent from the specific list.
+
+Subscription to incident specific lists
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Subscription is handled by the response teams. Disclosed parties who want
+to participate in the communication send a list of potential subscribers to
+the response team so the response team can validate subscription requests.
+
+Each subscriber needs to send a subscription request to the response team
+by email. The email must be signed with the subscriber's PGP key or S/MIME
+certificate. If a PGP key is used, it must be available from a public key
+server and is ideally connected to the Linux kernel's PGP web of trust. See
+also: https://www.kernel.org/signature.html.
+
+The response team verifies that the subscriber request is valid and adds
+the subscriber to the list. After subscription the subscriber will receive
+email from the mailing-list which is signed either with the list's PGP key
+or the list's S/MIME certificate. The subscriber's email client can extract
+the PGP key or the S/MIME certificate from the signature so the subscriber
+can send encrypted email to the list.
+
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -33,6 +33,7 @@ problems and bugs in particular.
reporting-bugs
security-bugs
+ embargoed-hardware-issues
bug-hunting
bug-bisect
tainted-kernels
^ permalink raw reply
* Re: [PATCH 15/22] docs: index.rst: don't use genindex for pdf output
From: Vinod Koul @ 2019-07-25 13:44 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jonathan Corbet, Sanyog Kale, Pierre-Louis Bossart,
David S. Miller, Jaroslav Kysela, Takashi Iwai, linux-doc,
dmaengine, alsa-devel, netdev
In-Reply-To: <45d57666e5738a0b85e948b0e94151fe1b1f9274.1563792334.git.mchehab+samsung@kernel.org>
On 22-07-19, 08:07, Mauro Carvalho Chehab wrote:
> The genindex logic is meant to be used only for html output, as
> pdf build has its own way to generate indexes.
> Documentation/driver-api/dmaengine/index.rst | 2 +-
> Documentation/driver-api/soundwire/index.rst | 2 +-
For dmaengine and soundwire:
Acked-by: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* [PATCH v6 0/2] arm64 relaxed ABI
From: Vincenzo Frascino @ 2019-07-25 13:50 UTC (permalink / raw)
To: linux-arm-kernel, linux-doc, linux-mm, linux-arch,
linux-kselftest, linux-kernel
Cc: vincenzo.frascino, Catalin Marinas, Will Deacon, Andrey Konovalov,
Szabolcs Nagy
In-Reply-To: <cover.1563904656.git.andreyknvl@google.com>
On arm64 the TCR_EL1.TBI0 bit has been always enabled on the arm64 kernel,
hence the userspace (EL0) is allowed to set a non-zero value in the top
byte but the resulting pointers are not allowed at the user-kernel syscall
ABI boundary.
This patchset proposes a relaxation of the ABI with which it is possible
to pass tagged tagged pointers to the syscalls, when these pointers are in
memory ranges obtained as described in tagged-address-abi.txt contained in
this patch series.
Since it is not desirable to relax the ABI to allow tagged user addresses
into the kernel indiscriminately, this patchset documents a new sysctl
interface (/proc/sys/abi/tagged_addr) that is used to prevent the applications
from enabling the relaxed ABI and a new prctl() interface that can be used to
enable or disable the relaxed ABI.
This patchset should be merged together with [1].
[1] https://patchwork.kernel.org/cover/10674351/
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
CC: Andrey Konovalov <andreyknvl@google.com>
Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Vincenzo Frascino (2):
arm64: Define Documentation/arm64/tagged-address-abi.rst
arm64: Relax Documentation/arm64/tagged-pointers.rst
Documentation/arm64/tagged-address-abi.rst | 148 +++++++++++++++++++++
Documentation/arm64/tagged-pointers.rst | 23 +++-
2 files changed, 164 insertions(+), 7 deletions(-)
create mode 100644 Documentation/arm64/tagged-address-abi.rst
--
2.22.0
^ permalink raw reply
* [PATCH v6 2/2] arm64: Relax Documentation/arm64/tagged-pointers.rst
From: Vincenzo Frascino @ 2019-07-25 13:50 UTC (permalink / raw)
To: linux-arm-kernel, linux-doc, linux-mm, linux-arch,
linux-kselftest, linux-kernel
Cc: vincenzo.frascino, Catalin Marinas, Will Deacon, Andrey Konovalov,
Szabolcs Nagy
In-Reply-To: <20190725135044.24381-1-vincenzo.frascino@arm.com>
On arm64 the TCR_EL1.TBI0 bit has been always enabled hence
the userspace (EL0) is allowed to set a non-zero value in the
top byte but the resulting pointers are not allowed at the
user-kernel syscall ABI boundary.
With the relaxed ABI proposed in this set, it is now possible to pass
tagged pointers to the syscalls, when these pointers are in memory
ranges obtained by an anonymous (MAP_ANONYMOUS) mmap().
Relax the requirements described in tagged-pointers.rst to be compliant
with the behaviours guaranteed by the ARM64 Tagged Address ABI.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
CC: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Acked-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
---
Documentation/arm64/tagged-pointers.rst | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/Documentation/arm64/tagged-pointers.rst b/Documentation/arm64/tagged-pointers.rst
index 2acdec3ebbeb..933aaef8d52f 100644
--- a/Documentation/arm64/tagged-pointers.rst
+++ b/Documentation/arm64/tagged-pointers.rst
@@ -20,7 +20,8 @@ Passing tagged addresses to the kernel
--------------------------------------
All interpretation of userspace memory addresses by the kernel assumes
-an address tag of 0x00.
+an address tag of 0x00, unless the userspace opts-in the ARM64 Tagged
+Address ABI via the PR_SET_TAGGED_ADDR_CTRL prctl().
This includes, but is not limited to, addresses found in:
@@ -33,18 +34,23 @@ This includes, but is not limited to, addresses found in:
- the frame pointer (x29) and frame records, e.g. when interpreting
them to generate a backtrace or call graph.
-Using non-zero address tags in any of these locations may result in an
-error code being returned, a (fatal) signal being raised, or other modes
-of failure.
+Using non-zero address tags in any of these locations when the
+userspace application did not opt-in to the ARM64 Tagged Address ABI
+may result in an error code being returned, a (fatal) signal being raised,
+or other modes of failure.
-For these reasons, passing non-zero address tags to the kernel via
-system calls is forbidden, and using a non-zero address tag for sp is
-strongly discouraged.
+For these reasons, when the userspace application did not opt-in, passing
+non-zero address tags to the kernel via system calls is forbidden, and using
+a non-zero address tag for sp is strongly discouraged.
Programs maintaining a frame pointer and frame records that use non-zero
address tags may suffer impaired or inaccurate debug and profiling
visibility.
+A definition of the meaning of ARM64 Tagged Address ABI and of the
+guarantees that the ABI provides when the userspace opts-in via prctl()
+can be found in: Documentation/arm64/tagged-address-abi.rst.
+
Preserving tags
---------------
@@ -59,6 +65,9 @@ be preserved.
The architecture prevents the use of a tagged PC, so the upper byte will
be set to a sign-extension of bit 55 on exception return.
+These behaviours are preserved even when the userspace opts-in to the ARM64
+Tagged Address ABI via the PR_SET_TAGGED_ADDR_CTRL prctl().
+
Other considerations
--------------------
--
2.22.0
^ permalink raw reply related
* [PATCH v6 1/2] arm64: Define Documentation/arm64/tagged-address-abi.rst
From: Vincenzo Frascino @ 2019-07-25 13:50 UTC (permalink / raw)
To: linux-arm-kernel, linux-doc, linux-mm, linux-arch,
linux-kselftest, linux-kernel
Cc: vincenzo.frascino, Catalin Marinas, Will Deacon, Andrey Konovalov,
Szabolcs Nagy
In-Reply-To: <20190725135044.24381-1-vincenzo.frascino@arm.com>
On arm64 the TCR_EL1.TBI0 bit has been always enabled hence
the userspace (EL0) is allowed to set a non-zero value in the
top byte but the resulting pointers are not allowed at the
user-kernel syscall ABI boundary.
With the relaxed ABI proposed through this document, it is now possible
to pass tagged pointers to the syscalls, when these pointers are in
memory ranges obtained by an anonymous (MAP_ANONYMOUS) mmap().
This change in the ABI requires a mechanism to requires the userspace
to opt-in to such an option.
Specify and document the way in which sysctl and prctl() can be used
in combination to allow the userspace to opt-in this feature.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
CC: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Acked-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
---
Documentation/arm64/tagged-address-abi.rst | 148 +++++++++++++++++++++
1 file changed, 148 insertions(+)
create mode 100644 Documentation/arm64/tagged-address-abi.rst
diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
new file mode 100644
index 000000000000..a8ecb991de82
--- /dev/null
+++ b/Documentation/arm64/tagged-address-abi.rst
@@ -0,0 +1,148 @@
+========================
+ARM64 TAGGED ADDRESS ABI
+========================
+
+Author: Vincenzo Frascino <vincenzo.frascino@arm.com>
+
+Date: 25 July 2019
+
+This document describes the usage and semantics of the Tagged Address
+ABI on arm64.
+
+1. Introduction
+---------------
+
+On arm64 the TCR_EL1.TBI0 bit has always been enabled on the kernel, hence
+the userspace (EL0) is entitled to perform a user memory access through a
+64-bit pointer with a non-zero top byte but the resulting pointers are not
+allowed at the user-kernel syscall ABI boundary.
+
+This document describes a relaxation of the ABI that makes it possible to
+to pass tagged pointers to the syscalls, when these pointers are in memory
+ranges obtained as described in section 2.
+
+Since it is not desirable to relax the ABI to allow tagged user addresses
+into the kernel indiscriminately, arm64 provides a new sysctl interface
+(/proc/sys/abi/tagged_addr) that is used to prevent the applications from
+enabling the relaxed ABI and a new prctl() interface that can be used to
+enable or disable the relaxed ABI.
+A detailed description of the newly introduced mechanisms will be provided
+in section 2.
+
+2. ARM64 Tagged Address ABI
+---------------------------
+
+From the kernel syscall interface perspective, we define, for the purposes
+of this document, a "valid tagged pointer" as a pointer that either has a
+zero value set in the top byte or has a non-zero value, is in memory ranges
+privately owned by a userspace process and is obtained in one of the
+following ways:
+- mmap() done by the process itself, where either:
+
+ - flags have **MAP_PRIVATE** and **MAP_ANONYMOUS**
+ - flags have **MAP_PRIVATE** and the file descriptor refers to a regular
+ file or **/dev/zero**
+
+- brk() system call done by the process itself (i.e. the heap area between
+ the initial location of the program break at process creation and its
+ current location).
+- any memory mapped by the kernel in the process's address space during
+ creation and with the same restrictions as for mmap() (e.g. data, bss,
+ stack).
+
+The ARM64 Tagged Address ABI is an opt-in feature, and an application can
+control it using the following:
+
+- **/proc/sys/abi/tagged_addr**: a new sysctl interface that can be used to
+ prevent the applications from enabling the access to the relaxed ABI.
+ The sysctl supports the following configuration options:
+
+ - **0**: Disable the access to the ARM64 Tagged Address ABI for all
+ the applications.
+ - **1** (Default): Enable the access to the ARM64 Tagged Address ABI for
+ all the applications.
+
+ If the access to the ARM64 Tagged Address ABI is disabled at a certain
+ point in time, all the applications that were using tagging before this
+ event occurs, will continue to use tagging.
+- **prctl()s**:
+
+ - **PR_SET_TAGGED_ADDR_CTRL**: Invoked by a process, can be used to enable or
+ disable its access to the ARM64 Tagged Address ABI.
+
+ The (unsigned int) arg2 argument is a bit mask describing the control mode
+ used:
+
+ - **PR_TAGGED_ADDR_ENABLE**: Enable ARM64 Tagged Address ABI.
+
+ The prctl(PR_SET_TAGGED_ADDR_CTRL, ...) will return -EINVAL if the ARM64
+ Tagged Address ABI is not available.
+
+ The arguments arg3, arg4, and arg5 are ignored.
+ - **PR_GET_TAGGED_ADDR_CTRL**: can be used to check the status of the Tagged
+ Address ABI.
+
+ The arguments arg2, arg3, arg4, and arg5 are ignored.
+
+The ABI properties set by the mechanisms described above are inherited by threads
+of the same application and fork()'ed children but cleared by execve().
+
+When a process has successfully opted into the new ABI by invoking
+PR_SET_TAGGED_ADDR_CTRL prctl(), this guarantees the following behaviours:
+
+ - Every currently available syscall, except the cases mentioned in section 3, can
+ accept any valid tagged pointer. The same rule is applicable to any syscall
+ introduced in the future.
+ - If a non valid tagged pointer is passed to a syscall then the behaviour
+ is undefined.
+ - Every valid tagged pointer is expected to work as an untagged one.
+ - The kernel preserves any valid tagged pointer and returns it to the
+ userspace unchanged (i.e. on syscall return) in all the cases except the
+ ones documented in the "Preserving tags" section of tagged-pointers.txt.
+
+A definition of the meaning of tagged pointers on arm64 can be found in:
+Documentation/arm64/tagged-pointers.txt.
+
+3. ARM64 Tagged Address ABI Exceptions
+--------------------------------------
+
+The behaviours described in section 2, with particular reference to the
+acceptance by the syscalls of any valid tagged pointer are not applicable
+to the following cases:
+
+ - mmap() addr parameter.
+ - mremap() new_address parameter.
+ - prctl(PR_SET_MM, PR_SET_MM_MAP, ...) struct prctl_mm_map fields.
+ - prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE, ...) struct prctl_mm_map fields.
+
+Any attempt to use non-zero tagged pointers will lead to undefined behaviour.
+
+4. Example of correct usage
+---------------------------
+.. code-block:: c
+
+ void main(void)
+ {
+ static int tbi_enabled = 0;
+ unsigned long tag = 0;
+
+ char *ptr = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS, -1, 0);
+
+ if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE,
+ 0, 0, 0) == 0)
+ tbi_enabled = 1;
+
+ if (ptr == (void *)-1) /* MAP_FAILED */
+ return -1;
+
+ if (tbi_enabled)
+ tag = rand() & 0xff;
+
+ ptr = (char *)((unsigned long)ptr | (tag << TAG_SHIFT));
+
+ *ptr = 'a';
+
+ ...
+ }
+
--
2.22.0
^ permalink raw reply related
* Re: [PATCH] hung_task: Allow printing warnings every check interval
From: Dmitry Safonov @ 2019-07-25 14:25 UTC (permalink / raw)
To: Tetsuo Handa, linux-kernel
Cc: Dmitry Safonov, Andrew Morton, Dmitry Vyukov, Ingo Molnar,
Jonathan Corbet, Thomas Gleixner, Peter Zijlstra (Intel),
Vasiliy Khoruzhick, linux-doc, linux-fsdevel
In-Reply-To: <2964b430-63d6-e172-84e2-cb269cf43443@i-love.sakura.ne.jp>
On 7/25/19 11:38 AM, Tetsuo Handa wrote:
> On 2019/07/25 2:02, Dmitry Safonov wrote:
>> Hung task detector has one timeout and has two associated actions on it:
>> - issuing warnings with names and stacks of blocked tasks
>> - panic()
>>
>> We want switches to panic (and reboot) if there's a task
>> in uninterruptible sleep for some minutes - at that moment something
>> ugly has happened and the box needs a reboot.
>> But we also want to detect conditions that are "out of range"
>> or approaching the point of failure. Under such conditions we want
>> to issue an "early warning" of an impending failure, minutes before
>> the switch is going to panic.
>
> Can't we do it by extending sysctl_hung_task_panic to accept values larger
> than 1, and decrease by one when at least one thread was reported by each
> check_hung_uninterruptible_tasks() check, and call panic() when
> sysctl_hung_task_panic reached to 0 (or maybe 1 is simpler) ?
>
> Hmm, might have the same problem regarding how/when to reset the counter.
> If some userspace process can reset the counter, such process can trigger
> SysRq-c when some period expired...
Yes, also current distributions already using the counter to print
warnings number of times and then silently ignore. I.e., on my Arch
Linux setup:
hung_task_warnings:10
>> It seems rather easy to add printing tasks and their stacks for
>> notification and debugging purposes into hung task detector without
>> complicating the code or major cost (prints are with KERN_INFO loglevel
>> and so don't go on console, only into dmesg log).
>
> Well, I don't think so. Might be noisy for systems without "quiet" kernel
> command line option, and we can't pass KERN_DEBUG to e.g. sched_show_task()...
Yes, that's why it's disabled by default (=0).
I tend to agree that printing with KERN_DEBUG may be better, but in my
point of view the patch isn't enough justification for patching
sched_show_task() and show_stack().
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH v10 4/5] overlayfs: internal getxattr operations without sepolicy checking
From: Mark Salyzyn @ 2019-07-25 14:37 UTC (permalink / raw)
To: Amir Goldstein
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <CAOQ4uxhtASSymEOdh4XByXbxWO2_ZivzqjBrgK7jB3fWXLqr_w@mail.gmail.com>
Thanks for the review.
On 7/25/19 4:00 AM, Amir Goldstein wrote:
> On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
>> Check impure, opaque, origin & meta xattr with no sepolicy audit
>> (using __vfs_getxattr) since these operations are internal to
>> overlayfs operations and do not disclose any data. This became
>> an issue for credential override off since sys_admin would have
>> been required by the caller; whereas would have been inherently
>> present for the creator since it performed the mount.
>>
>> This is a change in operations since we do not check in the new
>> ovl_vfs_getxattr function if the credential override is off or
>> not. Reasoning is that the sepolicy check is unnecessary overhead,
>> especially since the check can be expensive.
> I don't know that this reasoning suffice to skip the sepolicy checks
> for overlayfs private xattrs.
> Can't sepolicy be defined to allow get access to trusted.overlay.*?
Because for override credentials off, _everyone_ would need it (at least
on Android, the sole user AFAIK, and only on userdebug builds, not user
builds), and if everyone is special, and possibly including the random
applications we add from the play store, then no one is ...
For the override credentials on, the sepolicy would be required to add
to init or other mounters so that callers can actually use overlayfs.
Without the sepolicy for init, overlayfs will not function. the xattr
are in the backing storage and the details are not exported outside of
the driver. This would represent an imbalance since none of the callers
would require the sepolicy adjustment for the ;normal' case, but for
override credentials off as stated above, _everyone_ would require it.
Not against adding the sepolicy in Android, it is how we roll with only
opening up credentials on an as-need basis. We could deny it on user
(customer) builds and that closes a door that gains security. However
our people are starting to resist userdebug being different from user so
it may be a door I can not shut. Again felt like an imbalance for a
trusted driver read only operation.
Sincerely -- Mark Salyzyn
^ permalink raw reply
* Re: [PATCH v10 5/5] overlayfs: override_creds=off option bypass creator_cred
From: Mark Salyzyn @ 2019-07-25 14:38 UTC (permalink / raw)
To: Amir Goldstein
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <CAOQ4uxim8zZN5YHZs2OJz5A=3B0U10wyf371yadpe2B7hA8pZw@mail.gmail.com>
On 7/24/19 11:14 PM, Amir Goldstein wrote:
>> +void ovl_revert_creds(const struct cred *old_cred)
>> +{
>> + if (old_cred)
>> + revert_creds(old_cred);
>> +}
>> +
> Mark,
>
> Not sure if you have seen my "shutdown" patches:
> https://lore.kernel.org/linux-fsdevel/20190715133839.9878-4-amir73il@gmail.com/
Good to know!
>
> I am fine with this patch, but would like to request that you add @sb arg
> to the ovl_revert_creds() helper, so it is more useful for other things in the
> future that scope the underlying layers access (like shutdown).
Will respin and retest.
-- Mark
^ permalink raw reply
* Re: [PATCH v10 3/5] overlayfs: add __get xattr method
From: Mark Salyzyn @ 2019-07-25 15:03 UTC (permalink / raw)
To: Amir Goldstein
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <CAOQ4uxjizC1RhmLe3qmfASk2M-Y+QEiyLL1yJXa4zXAEby7Tig@mail.gmail.com>
On 7/24/19 10:48 PM, Amir Goldstein wrote:
> On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
>> Because of the overlayfs getxattr recursion, the incoming inode fails
>> to update the selinux sid resulting in avc denials being reported
>> against a target context of u:object_r:unlabeled:s0.
> This description is too brief for me to understand the root problem.
> What's wring with the overlayfs getxattr recursion w.r.t the selinux
> security model?
__vfs_getxattr (the way the security layer acquires the target sid
without recursing back to security to check the access permissions)
calls get xattr method, which in overlayfs calls vfs_getxattr on the
lower layer (which then recurses back to security to check permissions)
and reports back -EACCES if there was a denial (which is OK) and _no_
sid copied to caller's inode security data, bubbles back to the security
layer caller, which reports an invalid avc: message for
u:object_r:unlabeled:s0 (the uninitialized sid instead of the sid for
the lower filesystem target). The blocked access is 100% valid, it is
supposed to be blocked. This does however result in a cosmetic issue
that makes it impossible to use audit2allow to construct a rule that
would be usable to fix the access problem.
This problem would exist for any (in tree or out-of-tree) union
filesystems that need to reflect the __vfs_getxattr call into a
__vfs_getxattr call to the underlying filesystem.
>
> Please give an example of your unprivileged mounter use case
> to explain.
The mounter merely does not have access to the targets in one of the
referenced filesystems (for override creds = on)
In Android would be init, it does not have access to a subset of
u:object_r:*_exec:s0 and u::objects_r:vendor*:s0 labels. Based on a need
to access basis.
This gets worse if we add override creds = off, because the multitude of
callers could be denied access, and rightfully so, and we would have no
clue how to construct rules to grant them permissions using standard
tools like audit2allow.
I will figure out how to explain this in the commit message ... but do
tell me if I did not 'connect' you to the underlying problem so I can
make it clear to _everyone_.
>
> CC Vivek because I could really never understand all this.
>
>> Solution is to add a _get xattr method that calls the __vfs_getxattr
>> handler so that the context can be read in, rather than being denied
>> with an -EACCES when vfs_getxattr handler is called.
>> . . .
>> @@ -972,6 +989,7 @@ static const struct xattr_handler ovl_own_xattr_handler = {
>> static const struct xattr_handler ovl_other_xattr_handler = {
>> .prefix = "", /* catch all */
>> .get = ovl_other_xattr_get,
>> + .__get = __ovl_other_xattr_get,
>> .set = ovl_other_xattr_set,
>> };
>>
>
> Not very professional of me to comment on the proposed solution
> without understanding the problem, but my nose says this cannot
> be the right solution and if it is, then you better find a much better
> name for the API then __get() and document it properly.
Yes __get (instead of the existing get which checks sepolicy) was my
idea. get_wo_security was a close alternative.
We worked through 5 "solutions", this one privately appeared to please
the security folks. In fact the solution was their suggestion because
they noticed that __vfs_getxattr was meant to bypass sepolicy checking,
yet when nested through overlayfs, it called vfs_getxattr for the lower
filesystems and blocked necessary content (sid actually) from the upper
call in order to properly log the denial. I had originally copied just
the sid to the upper caller by adding layering violations that creeped
them out ;-/
^ permalink raw reply
* Re: [PATCH v10 3/5] overlayfs: add __get xattr method
From: Amir Goldstein @ 2019-07-25 15:43 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <af254162-10bf-1fc5-2286-8d002a287400@android.com>
On Thu, Jul 25, 2019 at 6:03 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> On 7/24/19 10:48 PM, Amir Goldstein wrote:
> > On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
> >> Because of the overlayfs getxattr recursion, the incoming inode fails
> >> to update the selinux sid resulting in avc denials being reported
> >> against a target context of u:object_r:unlabeled:s0.
> > This description is too brief for me to understand the root problem.
> > What's wring with the overlayfs getxattr recursion w.r.t the selinux
> > security model?
>
> __vfs_getxattr (the way the security layer acquires the target sid
> without recursing back to security to check the access permissions)
> calls get xattr method, which in overlayfs calls vfs_getxattr on the
> lower layer (which then recurses back to security to check permissions)
> and reports back -EACCES if there was a denial (which is OK) and _no_
> sid copied to caller's inode security data, bubbles back to the security
> layer caller, which reports an invalid avc: message for
> u:object_r:unlabeled:s0 (the uninitialized sid instead of the sid for
> the lower filesystem target). The blocked access is 100% valid, it is
> supposed to be blocked. This does however result in a cosmetic issue
> that makes it impossible to use audit2allow to construct a rule that
> would be usable to fix the access problem.
>
Ahhh you are talking about getting the security.selinux.* xattrs?
I was under the impression (Vivek please correct me if I wrong)
that overlayfs objects cannot have individual security labels and
the only way to label overlayfs objects is by mount options on the
entire mount? Or is this just for lower layer objects?
Anyway, the API I would go for is adding a @flags argument to
get() which can take XATTR_NOSECURITY akin to
FMODE_NONOTIFY, GFP_NOFS, meant to avoid recursions.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v10 4/5] overlayfs: internal getxattr operations without sepolicy checking
From: Amir Goldstein @ 2019-07-25 15:51 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <20df8497-17ea-27db-43c8-fcd73633e7f3@android.com>
On Thu, Jul 25, 2019 at 5:37 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> Thanks for the review.
>
> On 7/25/19 4:00 AM, Amir Goldstein wrote:
> > On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
> >> Check impure, opaque, origin & meta xattr with no sepolicy audit
> >> (using __vfs_getxattr) since these operations are internal to
> >> overlayfs operations and do not disclose any data. This became
> >> an issue for credential override off since sys_admin would have
> >> been required by the caller; whereas would have been inherently
> >> present for the creator since it performed the mount.
> >>
> >> This is a change in operations since we do not check in the new
> >> ovl_vfs_getxattr function if the credential override is off or
> >> not. Reasoning is that the sepolicy check is unnecessary overhead,
> >> especially since the check can be expensive.
> > I don't know that this reasoning suffice to skip the sepolicy checks
> > for overlayfs private xattrs.
> > Can't sepolicy be defined to allow get access to trusted.overlay.*?
>
> Because for override credentials off, _everyone_ would need it (at least
> on Android, the sole user AFAIK, and only on userdebug builds, not user
> builds), and if everyone is special, and possibly including the random
> applications we add from the play store, then no one is ...
>
OK. I am convinced.
One weak argument in favor of the patch:
ecryptfs also uses __vfs_getxattr for private xattrs.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v10 3/5] overlayfs: add __get xattr method
From: Mark Salyzyn @ 2019-07-25 16:22 UTC (permalink / raw)
To: Amir Goldstein
Cc: linux-kernel, kernel-team, Miklos Szeredi, Jonathan Corbet,
Vivek Goyal, Eric W . Biederman, Randy Dunlap, Stephen Smalley,
overlayfs, linux-doc
In-Reply-To: <CAOQ4uxi5S9HTx+wR1U_8vQ-6nyCozykWBZbZwiHhnXBGhXRz8Q@mail.gmail.com>
On 7/25/19 8:43 AM, Amir Goldstein wrote:
> On Thu, Jul 25, 2019 at 6:03 PM Mark Salyzyn <salyzyn@android.com> wrote:
>> On 7/24/19 10:48 PM, Amir Goldstein wrote:
>>> On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@android.com> wrote:
>>>> Because of the overlayfs getxattr recursion, the incoming inode fails
>>>> to update the selinux sid resulting in avc denials being reported
>>>> against a target context of u:object_r:unlabeled:s0.
>>> This description is too brief for me to understand the root problem.
>>> What's wring with the overlayfs getxattr recursion w.r.t the selinux
>>> security model?
>> __vfs_getxattr (the way the security layer acquires the target sid
>> without recursing back to security to check the access permissions)
>> calls get xattr method, which in overlayfs calls vfs_getxattr on the
>> lower layer (which then recurses back to security to check permissions)
>> and reports back -EACCES if there was a denial (which is OK) and _no_
>> sid copied to caller's inode security data, bubbles back to the security
>> layer caller, which reports an invalid avc: message for
>> u:object_r:unlabeled:s0 (the uninitialized sid instead of the sid for
>> the lower filesystem target). The blocked access is 100% valid, it is
>> supposed to be blocked. This does however result in a cosmetic issue
>> that makes it impossible to use audit2allow to construct a rule that
>> would be usable to fix the access problem.
>>
> Ahhh you are talking about getting the security.selinux.* xattrs?
> I was under the impression (Vivek please correct me if I wrong)
> that overlayfs objects cannot have individual security labels and
They can, and we _need_ them for Android's use cases, upper and lower
filesystems.
Some (most?) union filesystems (like Android's sdcardfs) set sepolicy
from the mount options, we did not need this adjustment there of course.
> the only way to label overlayfs objects is by mount options on the
> entire mount? Or is this just for lower layer objects?
>
> Anyway, the API I would go for is adding a @flags argument to
> get() which can take XATTR_NOSECURITY akin to
> FMODE_NONOTIFY, GFP_NOFS, meant to avoid recursions.
I do like it better (with the following 7 stages of grief below), best
for the future.
The change in this handler's API will affect all filesystem drivers
(well, my change affects the ABI, so it is not as-if I saved the world
from a module recompile) touching all filesystem sources with an even
larger audience of stakeholders. Larger audience of stakeholders, the
harder to get the change in ;-/. This is also concerning since I would
like this change to go to stable 4.4, 4.9, 4.14 and 4.19 where this
regression got introduced. I can either craft specific stable patches or
just let it go and deal with them in the android-common distributions
rather than seeking stable merged down. ABI/API breaks are a problem for
stable anyway ...
-- Mark
^ permalink raw reply
* Re: [PATCH v6] Documentation/checkpatch: Prefer stracpy over strcpy/strlcpy/strncpy.
From: Kees Cook @ 2019-07-25 18:50 UTC (permalink / raw)
To: NitinGote; +Cc: joe, corbet, akpm, apw, linux-doc, kernel-hardening
In-Reply-To: <20190725112219.6244-1-nitin.r.gote@intel.com>
On Thu, Jul 25, 2019 at 04:52:19PM +0530, NitinGote wrote:
> From: Nitin Gote <nitin.r.gote@intel.com>
>
> Added check in checkpatch.pl to deprecate strcpy(), strlcpy() and
> strncpy() in favor of stracpy().
stracpy() is preferred when the destination is a char array (rather than
a string pointer), so that likely needs to be clarified.
-Kees
>
> Updated Documentation/process/deprecated.rst for stracpy().
>
> Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
> ---
> Change log:
> v5->v6
> - Used stracpy() instead of strscpy().
>
> v4->v5
> - Change the subject line as per review comment.
> - v5 is Reviewed-by: Kees Cook <keescook@chromium.org>
>
> v3->v4
> - Removed "c:func:" from deprecated.rst as per review comment.
>
> v2->v3
> - Avoided use of $check in implementation.
> - Incorporated trivial comments.
>
> v1->v2
> - For string related apis, created different %deprecated_string_api
> and these will get emitted at CHECK Level using command line option
> -f/--file to avoid bad patched from novice script users.
>
> Documentation/process/deprecated.rst | 10 +++++-----
> scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
> 2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> index 49e0f64a3427..709662c71a1a 100644
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -84,7 +84,7 @@ buffer. This could result in linear overflows beyond the
> end of the buffer, leading to all kinds of misbehaviors. While
> `CONFIG_FORTIFY_SOURCE=y` and various compiler flags help reduce the
> risk of using this function, there is no good reason to add new uses of
> -this function. The safe replacement is :c:func:`strscpy`.
> +this function. The safe replacement is stracpy().
>
> strncpy() on NUL-terminated strings
> -----------------------------------
> @@ -93,9 +93,9 @@ will be NUL terminated. This can lead to various linear read overflows
> and other misbehavior due to the missing termination. It also NUL-pads the
> destination buffer if the source contents are shorter than the destination
> buffer size, which may be a needless performance penalty for callers using
> -only NUL-terminated strings. The safe replacement is :c:func:`strscpy`.
> -(Users of :c:func:`strscpy` still needing NUL-padding will need an
> -explicit :c:func:`memset` added.)
> +only NUL-terminated strings. In this case, the safe replacement is
> +stracpy(). If, however, the destination buffer still needs NUL-padding,
> +the safe replacement is stracpy_pad().
>
> If a caller is using non-NUL-terminated strings, :c:func:`strncpy()` can
> still be used, but destinations should be marked with the `__nonstring
> @@ -107,7 +107,7 @@ strlcpy()
> :c:func:`strlcpy` reads the entire source buffer first, possibly exceeding
> the given limit of bytes to copy. This is inefficient and can lead to
> linear read overflows if a source string is not NUL-terminated. The
> -safe replacement is :c:func:`strscpy`.
> +safe replacement is stracpy().
>
> Variable Length Arrays (VLAs)
> -----------------------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 342c7c781ba5..dddf5adf1aac 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) {
> }
> $deprecated_apis_search = "(?:${deprecated_apis_search})";
>
> +our %deprecated_string_apis = (
> + "strcpy" => "stracpy",
> + "strlcpy" => "stracpy",
> + "strncpy" => "stracpy - for non-NUL-terminated uses, strncpy dest should be __nonstring",
> +);
> +
> +#Create a search pattern for all these strings apis to speed up a loop below
> +our $deprecated_string_apis_search = "";
> +foreach my $entry (keys %deprecated_string_apis) {
> + $deprecated_string_apis_search .= '|' if ($deprecated_string_apis_search ne "");
> + $deprecated_string_apis_search .= $entry;
> +}
> +$deprecated_string_apis_search = "(?:${deprecated_string_apis_search})";
> +
> our $mode_perms_world_writable = qr{
> S_IWUGO |
> S_IWOTH |
> @@ -6446,6 +6460,16 @@ sub process {
> "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
> }
>
> +# check for string deprecated apis
> + if ($line =~ /\b($deprecated_string_apis_search)\b\s*\(/) {
> + my $deprecated_string_api = $1;
> + my $new_api = $deprecated_string_apis{$deprecated_string_api};
> + my $msg_level = \&WARN;
> + $msg_level = \&CHK if ($file);
> + &{$msg_level}("DEPRECATED_API",
> + "Deprecated use of '$deprecated_string_api', prefer '$new_api' instead\n" . $herecurr);
> + }
> +
> # check for various structs that are normally const (ops, kgdb, device_tree)
> # and avoid what seem like struct definitions 'struct foo {'
> if ($line !~ /\bconst\b/ &&
> --
> 2.17.1
>
--
Kees Cook
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox