linux-kernel - Re: [Linux-kernel-mentees] [PATCH net] AX.25: Fix out-of-bounds read in ax25

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: <20200723155057.GS2549@kadam>
Date:   Thu, 23 Jul 2020 18:50:58 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Peilin Ye <yepeilin.cs@...il.com>
Cc:     Joerg Reuter <jreuter@...na.de>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        syzkaller-bugs@...glegroups.com,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-hams@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Linux-kernel-mentees] [PATCH net] AX.25: Fix out-of-bounds read
 in ax25_connect()

On Thu, Jul 23, 2020 at 11:13:55AM -0400, Peilin Ye wrote:
> On Thu, Jul 23, 2020 at 05:28:15PM +0300, Dan Carpenter wrote:
> > On Wed, Jul 22, 2020 at 11:19:01AM -0400, Peilin Ye wrote:
> > > Checks on `addr_len` and `fsa->fsa_ax25.sax25_ndigis` are insufficient.
> > > ax25_connect() can go out of bounds when `fsa->fsa_ax25.sax25_ndigis`
> > > equals to 7 or 8. Fix it.
> > > 
> > > This issue has been reported as a KMSAN uninit-value bug, because in such
> > > a case, ax25_connect() reaches into the uninitialized portion of the
> > > `struct sockaddr_storage` statically allocated in __sys_connect().
> > > 
> > > It is safe to remove `fsa->fsa_ax25.sax25_ndigis > AX25_MAX_DIGIS` because
> > > `addr_len` is guaranteed to be less than or equal to
> > > `sizeof(struct full_sockaddr_ax25)`.
> > > 
> > > Reported-by: syzbot+c82752228ed975b0a623@...kaller.appspotmail.com
> > > Link: https://syzkaller.appspot.com/bug?id=55ef9d629f3b3d7d70b69558015b63b48d01af66
> > > Signed-off-by: Peilin Ye <yepeilin.cs@...il.com>
> > > ---
> > >  net/ax25/af_ax25.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> > > index fd91cd34f25e..ef5bf116157a 100644
> > > --- a/net/ax25/af_ax25.c
> > > +++ b/net/ax25/af_ax25.c
> > > @@ -1187,7 +1187,9 @@ static int __must_check ax25_connect(struct socket *sock,
> > >  	if (addr_len > sizeof(struct sockaddr_ax25) &&
> > >  	    fsa->fsa_ax25.sax25_ndigis != 0) {
> > >  		/* Valid number of digipeaters ? */
> > > -		if (fsa->fsa_ax25.sax25_ndigis < 1 || fsa->fsa_ax25.sax25_ndigis > AX25_MAX_DIGIS) {
> > > +		if (fsa->fsa_ax25.sax25_ndigis < 1 ||
> > > +		    addr_len < sizeof(struct sockaddr_ax25) +
> > > +		    sizeof(ax25_address) * fsa->fsa_ax25.sax25_ndigis) {
> > 
> > The "sizeof(ax25_address) * fsa->fsa_ax25.sax25_ndigis" can have an
> > integer overflow so you still need the
> > "fsa->fsa_ax25.sax25_ndigis > AX25_MAX_DIGIS" check.
> 
> Thank you for fixing this up! I did some math but I didn't think of
> that. Will be more careful when removing things.

No problem.  You had the right approach to look for ways to clean things
up.

Your patches make me happy because you're trying to fix important bugs.

regards,
dan carpenter