Re: [PATCH] xfs: fix up non-directory creation in SGID directories

From: Amir Goldstein <amir73il@gmail.com>
To: Varsha Teratipally <teratipally@google.com>
Cc: "Darrick J. Wong" <djwong@kernel.org>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	stable <stable@vger.kernel.org>, Christoph Hellwig <hch@lst.de>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Dave Chinner <david@fromorbit.com>,
	Yang Xu <xuyang2018.jy@fujitsu.com>
Subject: Re: [PATCH] xfs: fix up non-directory creation in SGID directories
Date: Wed, 7 Sep 2022 10:40:16 +0300	[thread overview]
Message-ID: <CAOQ4uxi_Q8aXUg+FM0Q9__t=KqJSVqOgkS8j8kNC3MQfniZLWA@mail.gmail.com> (raw)
In-Reply-To: <20220906183600.1926315-2-teratipally@google.com>

On Tue, Sep 6, 2022 at 9:36 PM Varsha Teratipally
<teratipally@google.com> wrote:
>
> From: Christoph Hellwig <hch@lst.de>
>
> XFS always inherits the SGID bit if it is set on the parent inode, while
> the generic inode_init_owner does not do this in a few cases where it can
> create a possible security problem, see commit 0fa3ecd87848
> ("Fix up non-directory creation in SGID directories") for details.
>
> Switch XFS to use the generic helper for the normal path to fix this,
> just keeping the simple field inheritance open coded for the case of the
> non-sgid case with the bsdgrpid mount option.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Hi Varsha,

For future reference, when posting an xfs patch for stable,
please follow these guidelines:

1. Post it to xfs list for review BEFORE posting to stable
2. LKML is not a relevant list
3. Tag the patch with the target kernel [PATCH 5.10]
4. Include the upstream commit id
5. Add some description (after --- line) about how you tested

Regarding this specific patch for 5.10, I had already tested and posted it
for review back in June [1].

Dave Chinner commented then that he was concerned about other
security issues discovered later on related to the generic implementation
of SGID stripping.
At the time, the generic upstream fixes and tests were still WIP.

Christoph Hellwig, the author of the original patch replied to Dave's
concern:

"To me backporting it seems good and useful, as it fixes a relatively
big problem.  The remaining issues seem minor compared to that."

Christiain Brauner who has been reviewing the generic upstream
also agreed that:

"Imho, backporting this patch is useful. It fixes a basic issue."

So this specific fix patch from the v5.12 release, which is not
relevant for 5.15.y has my blessing to go to 5.10.y.

Regardless, the last bits of the upstream work on the generic
implementation by Yang Xu have landed in v6.0-rc1 [2] and the
respective fstests have just recently landed in fstests v2022.09.04.

I already have all the patches backported to 5.10 [3] and will start
testing them in the following weeks, but now I also depend on Leah
to test them for 5.15.y before I can post to 5.10.y and that may take
a while...

Thanks,
Amir.

[1] https://lore.kernel.org/linux-xfs/CAOQ4uxg4=m9zEFbDAKXx7CP7HYiMwtsYSJvq076oKpy-OhK1uw@mail.gmail.com/
[2] https://lore.kernel.org/linux-fsdevel/20220809103957.1851931-1-brauner@kernel.org/
[3] https://github.com/amir73il/linux/commits/xfs-5.10.y-sgid-fixes