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.
Signed-off-by: Mark Salyzyn <[email protected]>
Cc: Miklos Szeredi <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Vivek Goyal <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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);
+}
+
struct super_block *ovl_same_sb(struct super_block *sb)
{
struct ovl_fs *ofs = sb->s_fs_info;
@@ -537,9 +543,9 @@ void ovl_copy_up_end(struct dentry *dentry)
bool ovl_check_origin_xattr(struct dentry *dentry)
{
- int res;
+ ssize_t res;
- res = vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
+ res = ovl_vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
/* Zero size value means "copied up but origin unknown" */
if (res >= 0)
@@ -550,13 +556,13 @@ bool ovl_check_origin_xattr(struct dentry *dentry)
bool ovl_check_dir_xattr(struct dentry *dentry, const char *name)
{
- int res;
+ ssize_t res;
char val;
if (!d_is_dir(dentry))
return false;
- res = vfs_getxattr(dentry, name, &val, 1);
+ res = ovl_vfs_getxattr(dentry, name, &val, 1);
if (res == 1 && val == 'y')
return true;
@@ -837,13 +843,13 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
int ovl_check_metacopy_xattr(struct dentry *dentry)
{
- int res;
+ ssize_t res;
/* Only regular files can have metacopy xattr */
if (!S_ISREG(d_inode(dentry)->i_mode))
return 0;
- res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+ res = ovl_vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
if (res < 0) {
if (res == -ENODATA || res == -EOPNOTSUPP)
return 0;
@@ -852,7 +858,7 @@ int ovl_check_metacopy_xattr(struct dentry *dentry)
return 1;
out:
- pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+ pr_warn_ratelimited("overlayfs: failed to get metacopy (%zi)\n", res);
return res;
}
@@ -878,7 +884,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
ssize_t res;
char *buf = 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 -ENODATA;
@@ -890,7 +896,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
if (!buf)
return -ENOMEM;
- res = vfs_getxattr(dentry, name, buf, res);
+ res = ovl_vfs_getxattr(dentry, name, buf, res);
if (res < 0)
goto fail;
}
--
2.22.0.657.g960e92d24f-goog
On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <[email protected]> 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 <[email protected]>
> Cc: Miklos Szeredi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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.
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 <[email protected]> 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
On Thu, Jul 25, 2019 at 5:37 PM Mark Salyzyn <[email protected]> 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 <[email protected]> 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.