netdev - Re: [PATCH v4] 9p: Add refcount to p9_req

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

Date:   Mon, 3 Sep 2018 06:36:20 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Tomas Bortoli <tomasbortoli@...il.com>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller@...glegroups.com,
        Dominique Martinet <dominique.martinet@....fr>
Subject: Re: [PATCH v4] 9p: Add refcount to p9_req_t

Tomas Bortoli wrote on Fri, Aug 31, 2018:
> On 08/30/2018 12:52 PM, Dominique Martinet wrote:
> > From: Tomas Bortoli <tomasbortoli@...il.com>
> > 
> > To avoid use-after-free(s), use a refcount to keep track of the
> > usable references to any instantiated struct p9_req_t.
> > 
> > This commit adds p9_req_put(), p9_req_get() and p9_req_try_get() as
> > wrappers to kref_put(), kref_get() and kref_get_unless_zero().
> > These are used by the client and the transports to keep track of
> > valid requests' references.
> > 
> > p9_free_req() is added back and used as callback by kref_put().
> > 
> > Add SLAB_TYPESAFE_BY_RCU as it ensures that the memory freed by
> > kmem_cache_free() will not be reused for another type until the rcu
> > synchronisation period is over, so an address gotten under rcu read
> > lock is safe to inc_ref() without corrupting random memory while
> > the lock is held.
> > 
> > Co-developed-by: Dominique Martinet <dominique.martinet@....fr>
> > Signed-off-by: Tomas Bortoli <tomasbortoli@...il.com>
> > Reported-by: syzbot+467050c1ce275af2a5b8@...kaller.appspotmail.com
> > Signed-off-by: Dominique Martinet <dominique.martinet@....fr>
> > ---
> > v3:
> >  - add req put if virtio zc request fails
> >  - add req put if cancelled callback is not defined for virtio
> >  - (incorrectly) add req put in rdma cancelled callback
> > 
> > v4:
> >  - removed rdma's cancelled callback put again
> >  - changed the else if no cancelled callback into actually giving virtio
> > a callback, xen does not need to call put in that case either because
> > both function rely on tag_lookup to find the request. trans_fd only
> > needs to put in cancelled because it also keeps the req in a list around
> > for cancel.
> >  - add req put for trans xen's request(), I'm not sure why that one was
> > missing either..
> > 
> > And with that I believe I am done testing all four transports.
> > I'll do a second round of tests next week just to make sure, but it
> > should be good enough™
> > Sorry for the multiple iterations.
>
> LGTM, thanks Dominique!

Thanks.

I've pushed this with the other patches to my '9p-next' branch, which
will get merged to linux-next today/tomorrow, so they can soak up some
syzbot testing as well.
That doesn't mean they cannot get reviews anymore, so don't be shy!


Tomas, I didn't see you reply about the 'rename req to rreq' requested
patch for trans_fd, but it's trivial so if you're not going to do it I
will submit something around next week.

-- 
Dominique