linux-kernel - Re: [PATCH] touchscreen: elants: fix a missing check of return values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20181222010030.GA31346@dtor-ws>
Date:   Fri, 21 Dec 2018 17:00:30 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Kangjie Lu <kjlu@....edu>
Cc:     Aditya Pakki <pakki001@....edu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Joe Perches <joe@...ches.com>, linux-input@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] touchscreen: elants: fix a missing check of return values

On Fri, Dec 21, 2018 at 03:05:29PM -0600, Kangjie Lu wrote:
> Hi Dmitry,
> 
> Thanks for the feedback.
> 
> On Fri, Dec 21, 2018 at 2:27 AM Dmitry Torokhov <dmitry.torokhov@...il.com>
> wrote:
> 
> > Hi Kangjie,
> >
> > On Fri, Dec 21, 2018 at 12:59:16AM -0600, Kangjie Lu wrote:
> > > elants_i2c_send() may fail, let's check its return values. The fix does
> > > the check and reports an error message upon the failure.
> > >
> > > Signed-off-by: Kangjie Lu <kjlu@....edu>
> > > ---
> > >  drivers/input/touchscreen/elants_i2c.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/input/touchscreen/elants_i2c.c
> > b/drivers/input/touchscreen/elants_i2c.c
> > > index f2cb23121833..cb3c1470bb68 100644
> > > --- a/drivers/input/touchscreen/elants_i2c.c
> > > +++ b/drivers/input/touchscreen/elants_i2c.c
> > > @@ -245,8 +245,14 @@ static int elants_i2c_calibrate(struct elants_data
> > *ts)
> > >       ts->state = ELAN_WAIT_RECALIBRATION;
> > >       reinit_completion(&ts->cmd_done);
> > >
> > > -     elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
> > > -     elants_i2c_send(client, rek, sizeof(rek));
> > > +     error = elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
> > > +     error |= elants_i2c_send(client, rek, sizeof(rek));
> >
> > I dislike this kind of error handling as this may result in invalid
> > error code being reported, in case 2 commands produce different results.
> >
> 
> I will fix this.
> 
> 
> > > +     if (error) {
> > > +             dev_err(&client->dev,
> > > +                             "error in sending I2C messages for
> > calibration: %d\n",
> > > +                             error);
> > > +             return error;
> >
> > If we just return like you do it here, interrupts will stay disabled and
> > touchscreen will be completely dead. With the old code we'd report
> > timeout on calibration, and touchscreen would have chance of working. We
> > would also be able to retry calibration.
> >
> 
> How about this: we print out the error message but still continue the
> following execution?

Yes, we could do dev_warn() here, but elants_i2c_send() already logs
errors, so I do not see much benefit from doing this.

> Also, if elants_i2c_send() fails,
> would wait_for_completion_interruptible_timeout() always capture a timeout?

Well, if controller does not get the calibration command(s) it will not
do anything and at worst in <timeout> time
wait_for_completion_interruptible_timeout() will return and we will
properly report this condition.

Another option is to rearrange the code to ensure we are not leaving
interrupts disabled on error.

Thanks.

-- 
Dmitry