linux-kernel - Re: [syzbot] WARNING in batadv_nc_mesh

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: <2056331.oJahCzYEoq@sven-desktop>
Date:   Sat, 23 Oct 2021 09:41:04 +0200
From:   Sven Eckelmann <sven@...fation.org>
To:     syzbot <syzbot+28b0702ada0bf7381f58@...kaller.appspotmail.com>,
        a@...table.cc, b.a.t.m.a.n@...ts.open-mesh.org,
        davem@...emloft.net, kuba@...nel.org, linux-kernel@...r.kernel.org,
        mareklindner@...mailbox.ch, netdev@...r.kernel.org,
        sw@...onwunderlich.de, syzkaller-bugs@...glegroups.com,
        Pavel Skripkin <paskripkin@...il.com>, linus.luessing@...3.blue
Subject: Re: [syzbot] WARNING in batadv_nc_mesh_free

On Friday, 22 October 2021 22:58:15 CEST Pavel Skripkin wrote:
[...]
> > Oh, ok. Next clean up call in batadv_nc_mesh_free() caused GPF, since
> > fields are not initialized. Let's try to clean up one by one and do not
> > break dependencies.
> > 
> > Quite ugly one, but idea is correct, I guess
> > 
> > Also, make each *_init() call clean up all allocated stuff to not call
> > corresponding *_free() on error handling path, since it introduces
> > problems, as syzbot reported

Thanks for the patch + syzbot interactions. I just wanted to implement a 
change - which would most likely have ended up the same way. Can you please 
send it to netdev and Cc b.a.t.m.a.n@...ts.open-mesh.org? We don't have 
anything else to submit at the moment for netdev and this patch can be applied 
by netdev directly. I will add my Acked-by in this process.

Not sure about the Fixes. It is definitely wrong in the initial commit.... but 
it got only really problematic when other features got introduced. I would 
still say that the initial one should be mentioned.

Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol")

@Linus, @Marek, @Antonio: Please check whether it is ok to move the 
batadv_v_mesh_init after batadv_tt_init + batadv_originator_init. 
batadv_v_mesh_init is basically there to initialize:

* bat_priv->bat_v.ogm_buff(|_len|_mutex)
* bat_priv->bat_v.ogm_seqno
* bat_priv->bat_v.ogm_wq

batadv_originator_init is there to initialize the 

* bat_priv->orig_hash
* bat_priv->orig_work (batadv_purge_orig) + queue it up

batadv_tt_init is a lot more complex but should in theory not interact with 
ogm specific algo ops.

I wouldn't know why there could be a problem but I would leave it to the 
experts.

Kind regards,
	Sven
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)