Date:   Wed, 5 Jan 2022 14:12:22 +0100
From:   Michal =?iso-8859-1?Q?Such=E1nek?= <msuchanek@suse.de>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Matthew Garrett <matthewgarrett@google.com>,
        James Morris <jmorris@namei.org>,
        David Howells <dhowells@redhat.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        acpi4asus-user@lists.sourceforge.net,
        platform-driver-x86@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH] debugfs: lockdown: Allow reading debugfs files that are
 not world readable
Message-ID: <20220105131222.GG117207@kunlun.suse.cz>
References: <20220104170505.10248-1-msuchanek@suse.de>
 <YdWGQ+Kxeo9Q7Kli@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YdWGQ+Kxeo9Q7Kli@kroah.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hello,

On Wed, Jan 05, 2022 at 12:51:31PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 04, 2022 at 06:05:05PM +0100, Michal Suchanek wrote:
> > 
> > When the kernel is locked down the kernel allows reading only debugfs
> > files with mode 444. Mode 400 is also valid but is not allowed.
> > 
> > Make the 444 into a mask.
> > 
> > Fixes: 5496197f9b08 ("debugfs: Restrict debugfs when the kernel is locked down")
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  fs/debugfs/file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Why has it taken so long for anyone to notice this (2 years!)?
> 
> Is that because no one uses the lockdown mode and tries to read debugfs
> files?

It's because people use those LTSS kernels that don't have this change.

> > 
> > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > index 7d162b0efbf0..950c63fa4d0b 100644
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -147,7 +147,7 @@ static int debugfs_locked_down(struct inode *inode,
> >  			       struct file *filp,
> >  			       const struct file_operations *real_fops)
> >  {
> > -	if ((inode->i_mode & 07777) == 0444 &&
> > +	if ((inode->i_mode & 07777 & ~0444) == 0 &&
> 
> You are now allowing more than just 0400, is that intentional?

The intent is to allow files that have permissions that are subset of
0444. The only one that makes sense and people complain about is 0400
but if you had 0440 or 0004 it would be permitted as well.

> I never understood why files that were 0666 were not able to be read
> here as well, why not allow that as well?  What was magic about 0444
> files?

I don't understand that either but I am not really trying to challenge
that part.

Thanks

Michal