Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 1 Aug 2021 23:24:21 +0300
From:   Petko Manolov <petkan@nucleusys.com>
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     davem@davemloft.net, kuba@kernel.org, linux-usb@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        syzbot+02c9f70f3afae308464a@syzkaller.appspotmail.com
Subject: Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval
Message-ID: <YQcC9eOf5+MXZRsG@carbon>
Mail-Followup-To: Pavel Skripkin <paskripkin@gmail.com>,
        davem@davemloft.net, kuba@kernel.org, linux-usb@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        syzbot+02c9f70f3afae308464a@syzkaller.appspotmail.com
References: <20210730214411.1973-1-paskripkin@gmail.com>
 <YQaVS5UwG6RFsL4t@carbon>
 <20210801223513.06bede26@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210801223513.06bede26@gmail.com>
X-Spam_score: -1.0
X-Spam_bar: -
X-Spam_report: Spam detection software, running on the system "zzt.nucleusys.com",
 has NOT identified this incoming email as spam.  The original
 message has been attached to this so you can view it or label
 similar future email.  If you have any questions, see
 @@CONTACT_ADDRESS@@ for details.
 Content preview:  On 21-08-01 22:35:13, Pavel Skripkin wrote: > On Sun, 1 Aug
    2021 15:36:27 +0300 Petko Manolov <petkan@nucleusys.com> wrote: > > > On
   21-07-31 00:44:11, Pavel Skripkin wrote: > > > Syzbot reported unin [...] 
 Content analysis details:   (-1.0 points, 5.0 required)
  pts rule name              description
 ---- ---------------------- --------------------------------------------------
 -1.0 ALL_TRUSTED            Passed through trusted hosts only via SMTP
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 21-08-01 22:35:13, Pavel Skripkin wrote:
> On Sun, 1 Aug 2021 15:36:27 +0300 Petko Manolov <petkan@nucleusys.com> wrote:
> 
> > On 21-07-31 00:44:11, Pavel Skripkin wrote:
> > > Syzbot reported uninit value pegasus_probe(). The problem was in missing
> > > error handling.
> > > 
> > > get_interrupt_interval() internally calls read_eprom_word() which can fail
> > > in some cases. For example: failed to receive usb control message. These
> > > cases should be handled to prevent uninit value bug, since
> > > read_eprom_word() will not initialize passed stack variable in case of
> > > internal failure.
> > 
> > Well, this is most definitelly a bug.
> > 
> > ACK!
> > 
> > 
> >		Petko
> 
> BTW: I found a lot uses of {get,set}_registers without error checking. I
> think, some of them could be fixed easily (like in enable_eprom_write), but, I
> guess, disable_eprom_write is not so easy. For example, if we cannot disable
> eprom should we retry? If not, will device get in some unexpected state?

Everything bracketed by PEGASUS_WRITE_EEPROM is more or less dead code.  I've
added this feature because the chip give us the ability to write to the flash,
but i seriously doubt anybody ever used it.  Come to think about it, i should
just remove this code.

> Im not familiar with this device, but I can prepare a patch to wrap all these
> calls with proper error checking

Well, i've stared at the code a bit and i see some places where not checking the
error returned by {get,set}_registers() could really be problematic.  I'll cook
a patch with what i think needs doing and will submit it here for review.


cheers,
Petko