linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	Miklos Szeredi <mszeredi@redhat.com>,
	linux-fsdevel@vger.kernel.org,
	overlayfs <linux-unionfs@vger.kernel.org>,
	LSM <linux-security-module@vger.kernel.org>,
	linux-kernel@vger.kernel.org,
	Christian Brauner <christian.brauner@ubuntu.com>
Subject: Re: [PATCH 2/2] security.capability: fix conversions on getxattr
Date: Fri, 29 Jan 2021 17:11:53 -0600	[thread overview]
Message-ID: <87h7mzs5hi.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20210129153807.GA1130@mail.hallyn.com> (Serge E. Hallyn's message of "Fri, 29 Jan 2021 09:38:07 -0600")

"Serge E. Hallyn" <serge@hallyn.com> writes:

> On Thu, Jan 28, 2021 at 08:44:26PM +0100, Miklos Szeredi wrote:
>> On Thu, Jan 28, 2021 at 6:09 PM Serge E. Hallyn <serge@hallyn.com> wrote:
>> >
>> > On Tue, Jan 19, 2021 at 07:34:49PM -0600, Eric W. Biederman wrote:
>> > > Miklos Szeredi <mszeredi@redhat.com> writes:
>> > >
>> > > >     if (!rootid_owns_currentns(kroot)) {
>> > > > -           kfree(tmpbuf);
>> > > > -           return -EOPNOTSUPP;
>> > > > +           size = -EOVERFLOW;
>> >
>> > Why this change?  Christian (cc:d) noticed that this is a user visible change.
>> > Without this change, if you are in a userns which has different rootid, the
>> > EOVERFLOW tells vfs_getxattr to vall back to __vfs_getxattr() and so you can
>> > see the v3 capability with its rootid.
>> >
>> > With this change, you instead just get EOVERFLOW.
>> 
>> Why would the user want to see nonsense (in its own userns) rootid and
>> what would it do with it?
>
> They would know that the data is there.

But an error of -EOVERFLOW still indicates data is there.
You just don't get the data because it can not be represented.

>> Please give an example where an untranslatable rootid would make any
>> sense at all to the user.
>
> I may have accidentally, from init_user_ns, as uid 1000, set an
> fscap with rootid 100001 instead of 100000, and wonder why the
> cap is not working in the container where 100000 is root.

Getting -EOVERFLOW when attempting to read the cap from inside
the user namespace will immediately tell you what is wrong. The rootid
does not map.

That is how all the non-mapping situations are handled.  Either
-EOVERFLOW or returning INVALID_UID/the unmapped user id aka nobody.

The existing code is wrong because it returns a completely untranslated
uid, which is completely non-sense.

An argument could be made for returning a rootid of 0xffffffff aka
INVALID_UID in a v3 cap xattr when the rootid can not be mapped.  I
think that is what we do with posix_acls that contain ids that don't
map.  My sense is returning -EOVERFLOW inside the container and
returning the v3 cap xattr outside the container will most quickly get
the problem diagnosed, and will be the most likely to not cause
problems.

If there is a good case for returning a v3 cap with rootid of 0xffffffff
instead of -EOVERFLOW we can do that.  Right now I don't see anything
that would be compelling in either direction.

Eric





  parent reply	other threads:[~2021-01-29 23:14 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-19 16:22 [PATCH 0/2] capability conversion fixes Miklos Szeredi
2021-01-19 16:22 ` [PATCH 1/2] ecryptfs: fix uid translation for setxattr on security.capability Miklos Szeredi
2021-01-19 21:06   ` Eric W. Biederman
2021-01-20  7:52     ` Miklos Szeredi
2021-01-22 16:04       ` Tyler Hicks
2021-01-22 18:31   ` Tyler Hicks
2021-01-25 13:25     ` Miklos Szeredi
2021-01-25 13:46       ` Miklos Szeredi
2021-01-26  1:52       ` Tyler Hicks
2021-01-19 16:22 ` [PATCH 2/2] security.capability: fix conversions on getxattr Miklos Szeredi
2021-01-20  1:34   ` Eric W. Biederman
2021-01-20  7:58     ` Miklos Szeredi
2021-01-28 16:58     ` Serge E. Hallyn
2021-01-28 20:19       ` Eric W. Biederman
2021-01-28 20:38         ` Miklos Szeredi
2021-01-28 20:49           ` Eric W. Biederman
     [not found]         ` <20210129154839.GC1130@mail.hallyn.com>
2021-01-29 22:55           ` Eric W. Biederman
2021-01-30  2:06             ` Serge E. Hallyn
2021-01-31 18:14               ` Eric W. Biederman
     [not found]       ` <CAJfpegt34fO8tUw8R2_ZxxKHBdBO_-quf+-f3N8aZmS=1oRdvQ@mail.gmail.com>
     [not found]         ` <20210129153807.GA1130@mail.hallyn.com>
2021-01-29 23:11           ` Eric W. Biederman [this message]
2021-01-30  2:04             ` Serge E. Hallyn
2021-01-20 19:37   ` kernel test robot
2021-01-20 21:08   ` kernel test robot
2021-01-19 21:10 ` [PATCH 0/2] capability conversion fixes Eric W. Biederman
2021-01-20  7:39   ` Miklos Szeredi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87h7mzs5hi.fsf@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    --cc=mszeredi@redhat.com \
    --cc=serge@hallyn.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).