linux-kernel - Re: [PATCH 4.19 03/54] sctp: fix refcount bug in sctp

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: <20200411184251.GM3625@localhost.localdomain>
Date:   Sat, 11 Apr 2020 15:42:51 -0300
From:   Marcelo Ricardo Leitner <mleitner@...hat.com>
To:     Pavel Machek <pavel@...x.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Qiujun Huang <hqjagain@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        syzbot+cea71eec5d6de256d54d@...kaller.appspotmail.com
Subject: Re: [PATCH 4.19 03/54] sctp: fix refcount bug in sctp_wfree

On Sat, Apr 11, 2020 at 08:28:13PM +0200, Pavel Machek wrote:
> Hi!
> 
> > The following case cause the bug:
> > for the trouble SKB, it was in outq->transmitted list
> 
> Ok... but this is one hell of "interesting" code.
> 
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -165,29 +165,44 @@ static void sctp_clear_owner_w(struct sc
> >  	skb_orphan(chunk->skb);
> >  }
> >  
> > +#define traverse_and_process()	\
> > +do {				\
> > +	msg = chunk->msg;	\
> > +	if (msg == prev_msg)	\
> > +		continue;	\
> > +	list_for_each_entry(c, &msg->chunks, frag_list) {	\
> > +		if ((clear && asoc->base.sk == c->skb->sk) ||	\
> > +		    (!clear && asoc->base.sk != c->skb->sk))	\
> > +			cb(c);	\
> > +	}			\
> > +	prev_msg = msg;		\
> > +} while (0)
> 
> Should this be function?
> 
> Do you see how it does "continue"? But the do {} while(0) wrapper eats
> the continue. "break" would be more clear here, but they are really
> equivalent due to way this macro is used.
> 
> It is just very, very confusing.

Agree. The 'continue' itself slipped in on a refactoring and I didn't
notice the confusing aspect of it. Initially, the code was written
without the macro, and the continue refererred to the outter
list_for_each_entry().

  Marcelo