linux-kernel - Re: [V9fs-developer] [PATCH 1/2] net/9p: embed fcall in req to round down buffer allocs

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180731011256.GA30388@nautica>
Date:   Tue, 31 Jul 2018 03:12:56 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     piaojun <piaojun@...wei.com>
Cc:     v9fs-developer@...ts.sourceforge.net,
        linux-fsdevel@...r.kernel.org, Greg Kurz <groug@...d.org>,
        Matthew Wilcox <willy@...radead.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [V9fs-developer] [PATCH 1/2] net/9p: embed fcall in req to round
 down buffer allocs

piaojun wrote on Tue, Jul 31, 2018:
> This is really a *big* patch, but the modification seems no harm. And I
> suggest running testcases to cover this. Please see my comments below.

I'm always running tests, but more never hurt - please help ;)

For reference I'm running a subset of cthon04[1], ltp[2] and some custom
tests like these[3][4]

[1] https://fedorapeople.org/cgit/steved/public_git/cthon04.git/
[2] https://github.com/linux-test-project/ltp
[3] https://github.com/phdeniel/sigmund/blob/master/modules/allfs.inc#L208
[4] https://github.com/phdeniel/sigmund/blob/master/modules/allfs.inc#L251

> > [...]
> > @@ -263,13 +261,13 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
> >  	if (!req)
> >  		return NULL;
> >  
> > -	req->tc = p9_fcall_alloc(alloc_msize);
> > -	req->rc = p9_fcall_alloc(alloc_msize);
> > -	if (!req->tc || !req->rc)
> > +	if (p9_fcall_alloc(&req->tc, alloc_msize))
> > +		goto free;
> > +	if (p9_fcall_alloc(&req->rc, alloc_msize))
> >  		goto free;
> >  
> > -	p9pdu_reset(req->tc);
> > -	p9pdu_reset(req->rc);
> > +	p9pdu_reset(&req->tc);
> > +	p9pdu_reset(&req->rc);
> >  	req->status = REQ_STATUS_ALLOC;
> >  	init_waitqueue_head(&req->wq);
> >  	INIT_LIST_HEAD(&req->req_list);
> > @@ -281,7 +279,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
> >  				GFP_NOWAIT);
> >  	else
> >  		tag = idr_alloc(&c->reqs, req, 0, P9_NOTAG, GFP_NOWAIT);
> > -	req->tc->tag = tag;
> > +	req->tc.tag = tag;
> >  	spin_unlock_irq(&c->lock);
> >  	idr_preload_end();
> >  	if (tag < 0)
> > @@ -290,8 +288,8 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
> >  	return req;
> >  
> >  free:
> > -	kfree(req->tc);
> > -	kfree(req->rc);
> > +	kfree(req->tc.sdata);
> > +	kfree(req->rc.sdata);
> 
> I wonder if we will free a wild pointer as 'sdata' has not been initialized NULL.

Good point, it's possible to jump here if the first fcall_alloc failed
since this declustered the two allocations.

Please consider this added to the previous patch (I'll send a v2 after
this has had more time for review, you can find the amended commit in my
9p-test tree meanwhile):
-----8<-----------------------------
diff --git a/net/9p/client.c b/net/9p/client.c
index ba99a94a12c9..fe030ef1c076 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -262,7 +262,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
 		return NULL;
 
 	if (p9_fcall_alloc(&req->tc, alloc_msize))
-		goto free;
+		goto free_req;
 	if (p9_fcall_alloc(&req->rc, alloc_msize))
 		goto free;
 
@@ -290,6 +290,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
 free:
 	kfree(req->tc.sdata);
 	kfree(req->rc.sdata);
+free_req:
 	kmem_cache_free(p9_req_cache, req);
 	return ERR_PTR(-ENOMEM);
 }
-----8<-----------------------------

The second goto doesn't need changing because rc.sdata will be set to
NULL if the allocation failed

-- 
Dominique