Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 8 Jun 2022 11:13:34 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Jiri Slaby <jirislaby@kernel.org>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in
 con_get_unimap()
In-Reply-To: <e9f35a26-8f5b-f585-2e8c-19690bf3c770@kernel.org>
Message-ID: <10bf163-1b82-743-69f7-2d7a4a8245f5@linux.intel.com>
References: <20220607104946.18710-1-jslaby@suse.cz> <20220607104946.18710-16-jslaby@suse.cz> <8ad44d3b-449f-4a30-1a4c-dffd43bbbd1@linux.intel.com> <e9f35a26-8f5b-f585-2e8c-19690bf3c770@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-5759598-1654676018=:1676"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-5759598-1654676018=:1676
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT

On Wed, 8 Jun 2022, Jiri Slaby wrote:

> On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> > On Tue, 7 Jun 2022, Jiri Slaby wrote:
> > 
> > > Only the return value of copy_to_user() is checked in con_get_unimap().
> > > Do the same for put_user() of the count too.
> > > 
> > > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> > > ---
> > >   drivers/tty/vt/consolemap.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> > > index 831450f2bfd1..92b5dddb00d9 100644
> > > --- a/drivers/tty/vt/consolemap.c
> > > +++ b/drivers/tty/vt/consolemap.c
> > > @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct,
> > > ushort __user *uct,
> > >   	console_unlock();
> > >   	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
> > >   		ret = -EFAULT;
> > > -	put_user(ect, uct);
> > > +	if (put_user(ect, uct))
> > > +		ret = -EFAULT;
> > >   	kvfree(unilist);
> > >   	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
> > >   }
> > > 
> > 
> > Doesn't this fix something?
> 
> If you mean a Fixes tag, this is pre-git.
> 
> If you mean a bug, well, likely yes, users now get informed. But I don't think
> anyone cares ;).

Yes, I meant Fixes tag but I guess it's not important.

> But who knows, maybe we will start seeing userspace failures
> now (as they might not provide writable count field -- unlikely). That's one
> of the reasons why I did this as a separate commit. Let's see if are going to
> revert this or not...

Ok.

-- 
 i.

--8323329-5759598-1654676018=:1676--