Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 15 Jul 2018 11:05:44 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Todd Poynor <toddpoynor@gmail.com>
Cc:     Rob Springer <rspringer@google.com>,
        John Joseph <jnjoseph@google.com>,
        Ben Chan <benchan@chromium.org>, devel@driverdev.osuosl.org,
        Zhongze Hu <frankhu@chromium.org>,
        linux-kernel@vger.kernel.org, Simon Que <sque@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        Todd Poynor <toddpoynor@google.com>,
        Dmitry Torokhov <dtor@chromium.org>
Subject: Re: [PATCH 11/18] staging: gasket: always allow root open for write
Message-ID: <20180715090544.GC23333@kroah.com>
References: <20180714055816.223754-1-toddpoynor@gmail.com>
 <20180714055816.223754-12-toddpoynor@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180714055816.223754-12-toddpoynor@gmail.com>
User-Agent: Mutt/1.10.0 (2018-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 13, 2018 at 10:58:09PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Always allow root to open device for writing.
> 
> Drop special-casing of ioctl permissions for root vs. owner.
> 
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/apex_driver.c  |  9 +++------
>  drivers/staging/gasket/gasket_core.c  |  8 +++++---
>  drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
>  3 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> index b1318482ba65..ffe11d8168ea 100644
> --- a/drivers/staging/gasket/apex_driver.c
> +++ b/drivers/staging/gasket/apex_driver.c
> @@ -644,13 +644,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
>  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
>  {
>  	struct gasket_dev *gasket_dev = filp->private_data;
> -	int root = capable(CAP_SYS_ADMIN);
> -	int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> -		       current->tgid == gasket_dev->dev_info.ownership.owner;
> +	fmode_t write;
>  
> -	if (root || is_owner)
> -		return 1;
> -	return 0;
> +	write = filp->f_mode & FMODE_WRITE;

Ok, this is insane.  You don't change, or check, the permissions on a
file handle while it is already open, as you only check the permissions
on OPEN, not on WRITE.  See the recent rant from Linus on the linux-api
list for yet-another-long-threaad in which he explains this.

So this whole ioctl can just be removed, it is totally crazy and wrong
and should just be removed.

Want me to go remove it right now?

thanks,

greg k-h