linux-kernel - Re: [PATCH] 9p: fix crash when transaction killed

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: <Y4dcAGM+0xzOgSCa@codewreck.org>
Date:   Wed, 30 Nov 2022 22:34:56 +0900
From:   asmadeus@...ewreck.org
To:     Schspa Shi <schspa@...il.com>
Cc:     ericvh@...il.com, lucho@...kov.net, linux_oss@...debyte.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, v9fs-developer@...ts.sourceforge.net,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzbot+8f1060e2aaf8ca55220b@...kaller.appspotmail.com
Subject: Re: [PATCH] 9p: fix crash when transaction killed

Schspa Shi wrote on Wed, Nov 30, 2022 at 09:15:12PM +0800:
> >> If the req was newly alloced(It was at a new page), refcount maybe not
> >> 0, there will be problem in this case. It seems we can't relay on this.
> >> 
> >> We need to set the refcount to zero before add it to idr in p9_tag_alloc.
> >
> > Hmm, if it's reused then it's zero by definition, but if it's a new
> > allocation (uninitialized) then anything goes; that lookup could find
> > and increase it before the refcount_set, and we'd have an off by one
> > leading to use after free. Good catch!
> >
> > Initializing it to zero will lead to the client busy-looping until after
> > the refcount is properly set, which should work.
> 
> Why? It looks no different from the previous process here. Initializing
> it to zero should makes no difference.

I do not understand this remark.
If this is a freed request it will be zero, because we freed the request
as the refcount hit zero, but if it's a newly allocated request then the
memory is uninitalized, and the lookup can get anything.

In that case we want refcount to be zero to have the check in
p9_tag_lookup to not use the request until we set the refcount to 2.


> > Setting refcount early might have us use an re-used req before the tag
> > has been changed so that one cannot move.
> >
> > Could you test with just that changed if syzbot still reproduces this
> > bug? (perhaps add a comment if you send this)
> >
> 
> I have upload a new v2 change for this. But I can't easily reproduce
> this problem.

Ah, I read that v2 as you actually ran some tests with this, sorry for
the misuderstanding.

Well, it's a fix anyway, so it cannot hurt to apply...
-- 
Dominique