netdev - Re: [PATCH v4 5/5] r8152: Block future register access if register access fails

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>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 20 Oct 2023 14:10:42 -0700
From: Doug Anderson <dianders@...omium.org>
To: Hayes Wang <hayeswang@...ltek.com>
Cc: Jakub Kicinski <kuba@...nel.org>, "David S . Miller" <davem@...emloft.net>, 
	Grant Grundler <grundler@...omium.org>, Edward Hill <ecgh@...omium.org>, 
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>, Simon Horman <horms@...nel.org>, 
	Laura Nao <laura.nao@...labora.com>, Alan Stern <stern@...land.harvard.edu>, 
	Bjørn Mork <bjorn@...k.no>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 5/5] r8152: Block future register access if register
 access fails

Hi,

On Fri, Oct 20, 2023 at 8:42 AM Doug Anderson <dianders@...omium.org> wrote:
>
> > > @@ -8293,6 +8394,8 @@ static int rtl8152_post_reset(struct usb_interface *intf)
> > >         if (!tp)
> > >                 return 0;
> > >
> > > +       rtl_set_accessible(tp);
> > > +
> >
> > Excuse me. I have a new idea. You could check if it is possible.
> > If you remove test_bit(PROBED_WITH_NO_ERRORS, &tp->flags) in pre_reset(),
> > the driver wouldn't be unbound and rebound. Instead, you test PROBED_WITH_NO_ERRORS
> > here to re-initialize the device. Then, you could limit the times of USB reset, and
> > the infinite loop wouldn't occur. The code would be like the following,
> >
> >         if (!test_bit(PROBED_WITH_NO_ERRORS, &tp->flags)) {
> >                 /* re-init */
> >                 mutex_lock(&tp->control);
> >                 tp->rtl_ops.init(tp);
> >                 mutex_unlock(&tp->control);
> >                 rtl_hw_phy_work_func_t(&tp->hw_phy_work.work);
> >
> >                 /* re-open(). Maybe move after checking netif_running(netdev) */
> >                 mutex_lock(&tp->control);
> >                 tp->rtl_ops.up(tp);
> >                 mutex_unlock(&tp->control);
> >
> >                 /* check if there is any control error */
> >                 if (test_bit(RTL8152_INACCESSIBLE, &tp->flags) {
> >                         if (tp->reg_access_reset_count < REGISTER_ACCESS_MAX_RESETS) {
> >                                 /* queue reset again ? */
> >                         } else {
> >                                 ...
> >                         }
> >                         /* return 0 ? */
> >                 } else {
> >                         set_bit(PROBED_WITH_NO_ERRORS, &tp->flags)
> >                 }
> >         }
>
> The above solution worries me.
>
> I guess one part of this is that it replicates some logic that's in
> probe(). That's not necessarily awful, but we'd at least want to
> reorganize things so that they could share code if possible, though
> maybe that's hard to do with the extra grabs of the mutex?
>
> The other part that worries me is that in the core when we added the
> network device that something in the core might have cached bogus data
> about our network device. This doesn't seem wonderful to me.
>
> I guess yet another part is that your proposed solution there has a
> whole bunch of question marks on it. If it's not necessarily obvious
> what we should do in this case then it doesn't feel like a robust
> solution.
>
> It seems like your main concern here is with the potential for an
> infinite number of resets. I have sent up a small patch to the USB
> core [1] addressing this concern. Let's see what folks say about that
> patch. If it is accepted then it seems like we could just not worry
> about it. If it's not accepted then perhaps feedback on that patch
> will give us additional guidance.
>
> In the meantime I'll at least post v5 since I don't want to leave the
> patch up there with the mismatched mutex. I'll have my v5 point at my
> USB core patch.
>
> [1] https://lore.kernel.org/r/20231020083125.1.I3e5f7abcbf6f08d392e31a5826b7f234df662276@changeid

OK, Alan responded to the patch above and suggested simply putting the
retry in the probe routine itself. I think that's actually in the same
spirit as your suggestion but addresses the concerns that I had. I
coded it up and tested it and it seems to work, so I posted that as v5
[2]. Please take a look.

[2] https://lore.kernel.org/r/20231020210751.3415723-1-dianders@chromium.org