netdev - Re: [PATCH net-next] net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo

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

Message-ID: <Y42d2us5Pv1UqhEj@unreal>
Date:   Mon, 5 Dec 2022 09:29:30 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
Cc:     netdev@...r.kernel.org, nbd@....name, john@...ozen.org,
        sean.wang@...iatek.com, Mark-MC.Lee@...iatek.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, matthias.bgg@...il.com,
        linux-mediatek@...ts.infradead.org, lorenzo.bianconi@...hat.com
Subject: Re: [PATCH net-next] net: ethernet: mtk_wed: fix possible deadlock
 if mtk_wed_wo_init fails

On Sun, Dec 04, 2022 at 04:09:21PM +0100, Lorenzo Bianconi wrote:
> > On Fri, Dec 02, 2022 at 06:36:33PM +0100, Lorenzo Bianconi wrote:
> > > Introduce __mtk_wed_detach() in order to avoid a possible deadlock in
> > > mtk_wed_attach routine if mtk_wed_wo_init fails.
> > > 
> > > Fixes: 4c5de09eb0d0 ("net: ethernet: mtk_wed: add configure wed wo support")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > > ---
> > >  drivers/net/ethernet/mediatek/mtk_wed.c     | 24 ++++++++++++++-------
> > >  drivers/net/ethernet/mediatek/mtk_wed_mcu.c | 10 ++++++---
> > >  drivers/net/ethernet/mediatek/mtk_wed_wo.c  |  3 +++
> > >  3 files changed, 26 insertions(+), 11 deletions(-)
> > 
> > <...>
> > 
> > > diff --git a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > > index f9539e6233c9..b084009a32f9 100644
> > > --- a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > > +++ b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > > @@ -176,6 +176,9 @@ int mtk_wed_mcu_send_msg(struct mtk_wed_wo *wo, int id, int cmd,
> > >  	u16 seq;
> > >  	int ret;
> > >  
> > > +	if (!wo)
> > > +		return -ENODEV;
> > 
> > <...>
> > 
> > >  static void
> > >  mtk_wed_wo_hw_deinit(struct mtk_wed_wo *wo)
> > >  {
> > > +	if (!wo)
> > > +		return;
> > 
> > How are these changes related to the written in deadlock?
> > How is it possible to get internal mtk functions without valid wo?
> 
> Hi Leon,
> 
> if mtk_wed_rro_alloc() fails in mtk_wed_attach(), we will end up running
> __mtk_wed_detach() when wo struct is not allocated yet (wo is allocated in
> mtk_wed_wo_init()).

IMHO, it is a culprit, proper error unwind means that you won't call to
uninit functions for something that is not initialized yet. It is better
to fix it instead of adding "if (!wo) ..." checks.

> Moreover __mtk_wed_detach() can run mtk_wed_wo_reset() and mtk_wed_wo_deinit()

This is another side of same coin. If you can run them in parallel, you
need locking protection and ability to cancel work, so nothing is going
to be executed once cleanup succeeded.

These were my 2 cents, totally IMHO.

Thanks