sunrpc: don't pass on-stack memory to sg_set_buf
Subsystems (4)
PATCH 1/1 sunrpc: don't pass on-stack memory to sg_set_buf
From: "J. Bruce Fields" <[email protected]>
As of ac4e97abce9b "scatterlist: sg_set_buf() argument must be in linear mapping", sg_set_buf hits a BUG when make_checksum_v2->xdr_process_buf, among other callers, passes it memory on the stack.
We only need a scatterlist to pass this to the crypto code, and it seems like overkill to require kmalloc'd memory just to encrypt a few bytes, but for now this seems the best fix.
Note many of these callers are in the NFS write paths, so we shouldn't really be allocating GFP_KERNEL. But we already have other allocations in these code paths. A larger redesign may be necessary to allow allocations to be done earlier.
--- net/sunrpc/auth_gss/auth_gss.c | 13 ++++-- net/sunrpc/auth_gss/gss_krb5_crypto.c | 82 ++++++++++++++++++++--------------- net/sunrpc/auth_gss/svcauth_gss.c | 21 ++++++--- 3 files changed, 71 insertions(+), 45 deletions(-)
net/sunrpc/auth_gss/auth_gss.c +9 -4
@@ -1616,7 +1616,7 @@ gss_validate(struct rpc_task *task, __be32 *p) {struct rpc_cred *cred = task->tk_rqstp->rq_cred;struct gss_cl_ctx *ctx =gss_cred_get_ctx (cred); -__be32 seq; +__be32 *seq =NULL ;struct kvec iov;struct xdr_buf verf_buf;struct xdr_netobj mic;
@@ -1631,9 +1631,12 @@ gss_validate(struct rpc_task *task, __be32 *p) goto out_bad;if (flav !=RPC_AUTH_GSS ) goto out_bad; - seq =htonl (task->tk_rqstp->rq_seqno); - iov.iov_base = &seq; - iov.iov_len =sizeof (seq); + seq =kmalloc (4 ,GFP_KERNEL ); +if (!seq) + goto out_bad; + *seq =htonl (task->tk_rqstp->rq_seqno); + iov.iov_base = seq; + iov.iov_len =4 ;xdr_buf_from_iov (&iov, &verf_buf); mic.data = (u8 *)p; mic.len = len;
@@ -1653,11 +1656,13 @@ gss_validate(struct rpc_task *task, __be32 *p)gss_put_ctx (ctx);dprintk ("RPC: %5u %s: gss_verify_mic succeeded.\n" , task->tk_pid, __func__); +kfree (seq);return p +XDR_QUADLEN (len); out_bad:gss_put_ctx (ctx);dprintk ("RPC: %5u %s failed ret %ld.\n" , task->tk_pid, __func__,PTR_ERR (ret)); +kfree (seq);return ret; }
net/sunrpc/auth_gss/gss_krb5_crypto.c +48 -34
179 lines not on this page · raw message
net/sunrpc/auth_gss/svcauth_gss.c +14 -7
@@ -718,30 +718,37 @@ gss_write_null_verf(struct svc_rqst *rqstp)static int gss_write_verf (struct svc_rqst *rqstp,struct gss_ctx *ctx_id,u32 seq) { -__be32 xdr_seq; +__be32 *xdr_seq;u32 maj_stat;struct xdr_buf verf_data;struct xdr_netobj mic;__be32 *p;struct kvec iov; +int err =-1 ;svc_putnl (rqstp->rq_res.head,RPC_AUTH_GSS ); - xdr_seq =htonl (seq); + xdr_seq =kmalloc (4 ,GFP_KERNEL ); +if (!xdr_seq) +return -1 ; + *xdr_seq =htonl (seq); - iov.iov_base = &xdr_seq; - iov.iov_len =sizeof (xdr_seq); + iov.iov_base = xdr_seq; + iov.iov_len =4 ;xdr_buf_from_iov (&iov, &verf_data); p = rqstp->rq_res.head->iov_base + rqstp->rq_res.head->iov_len; mic.data = (u8 *)(p +1 ); maj_stat =gss_get_mic (ctx_id, &verf_data, &mic);if (maj_stat !=GSS_S_COMPLETE ) -return -1 ; + goto out; *p++ =htonl (mic.len);memset ((u8 *)p + mic.len,0 ,round_up_to_quad (mic.len) - mic.len); p +=XDR_QUADLEN (mic.len);if (!xdr_ressize_check (rqstp, p)) -return -1 ; -return 0 ; + goto out; + err =0 ; +out: +kfree (xdr_seq); +return err; }struct gss_domain {
Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
DQo+IE9uIE9jdCAyNSwgMjAxNiwgYXQgMTQ6NDUsIEouIEJydWNlIEZpZWxkcyA8YmZpZWxkc0Bm aWVsZHNlcy5vcmc+IHdyb3RlOg0KPiANCj4gRnJvbTogIkouIEJydWNlIEZpZWxkcyIgPGJmaWVs ZHNAcmVkaGF0LmNvbT4NCj4gDQo+IEFzIG9mIGFjNGU5N2FiY2U5YiAic2NhdHRlcmxpc3Q6IHNn X3NldF9idWYoKSBhcmd1bWVudCBtdXN0IGJlIGluIGxpbmVhcg0KPiBtYXBwaW5nIiwgc2dfc2V0 X2J1ZiBoaXRzIGEgQlVHIHdoZW4gbWFrZV9jaGVja3N1bV92Mi0+eGRyX3Byb2Nlc3NfYnVmLA0K PiBhbW9uZyBvdGhlciBjYWxsZXJzLCBwYXNzZXMgaXQgbWVtb3J5IG9uIHRoZSBzdGFjay4NCj4g DQo+IFdlIG9ubHkgbmVlZCBhIHNjYXR0ZXJsaXN0IHRvIHBhc3MgdGhpcyB0byB0aGUgY3J5cHRv IGNvZGUsIGFuZCBpdCBzZWVtcw0KPiBsaWtlIG92ZXJraWxsIHRvIHJlcXVpcmUga21hbGxvYydk IG1lbW9yeSBqdXN0IHRvIGVuY3J5cHQgYSBmZXcgYnl0ZXMsDQo+IGJ1dCBmb3Igbm93IHRoaXMg c2VlbXMgdGhlIGJlc3QgZml4Lg0KPiANCj4gTm90ZSBtYW55IG9mIHRoZXNlIGNhbGxlcnMgYXJl IGluIHRoZSBORlMgd3JpdGUgcGF0aHMsIHNvIHdlIHNob3VsZG4ndA0KPiByZWFsbHkgYmUgYWxs b2NhdGluZyBHRlBfS0VSTkVMLiAgQnV0IHdlIGFscmVhZHkgaGF2ZSBvdGhlciBhbGxvY2F0aW9u cw0KPiBpbiB0aGVzZSBjb2RlIHBhdGhzLiAgQSBsYXJnZXIgcmVkZXNpZ24gbWF5IGJlIG5lY2Vz c2FyeSB0byBhbGxvdw0KPiBhbGxvY2F0aW9ucyB0byBiZSBkb25lIGVhcmxpZXIuDQoNCk5BQ0vi gKYgIEkgYWdyZWUgdGhhdCB0aGVyZSBtYXkgYWxyZWFkeSBiZSBib3JrYWdlIGluIFJQQ1NFQ19H U1MtbGFuZCwgYnV0IHRoYXTigJlzIG5vdCBhIHJlYXNvbiB0byBiZSBhZGRpbmcgdG8gdGhlIHBp bGUgb2YgdGhpbmdzIHRoYXQgbmVlZCB0byBiZSBmaXhlZOKApiBUaGUgYWxsb2NhdGlvbnMgdGhh dCBsaWUgaW4gdGhlIFJQQyBjbGllbnTigJlzIGVuY29kZS9kZWNvZGUgcGF0aCBkbyBuZWVkIHRv IGJlIEdGUF9OT0ZT4oCmLg0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Trond Myklebust
Quoted 13 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
As of
ac4e97abce9b"scatterlist:sg_set_buf()argument must be in linear mapping",sg_set_bufhits a BUG whenmake_checksum_v2->xdr_process_buf, among other callers, passes it memory on the stack.We only need a scatterlist to pass this to the crypto code, and it seems like overkill to require kmalloc'd memory just to encrypt a few bytes, but for now this seems the best fix.
Quoted 6 lines
Quoted 4 lines
Note many of these callers are in the NFS write paths, so we shouldn't really be allocating
GFP_KERNEL. But we already have other allocations in these code paths. A larger redesign may be necessary to allow allocations to be done earlier.NACK… I agree that there may already be borkage in RPCSEC_GSS-land, but that’s not a reason to be adding to the pile of things that need to be fixed… The allocations that lie in the RPC client’s encode/decode path do need to be GFP_NOFS….
OK. Any disadvantage to keeping this patch with just GFP_NOFS allocations everywhere that might be in the write path?
--b.
A message that is not in the archive (sent off-list or not yet received): [email protected]
DQo+IE9uIE9jdCAyNSwgMjAxNiwgYXQgMTU6NTksIEZpZWxkcyBCcnVjZSBKYW1lcyA8YmZpZWxk c0BmaWVsZHNlcy5vcmc+IHdyb3RlOg0KPiANCj4gT24gVHVlLCBPY3QgMjUsIDIwMTYgYXQgMDc6 MzQ6NDNQTSArMDAwMCwgVHJvbmQgTXlrbGVidXN0IHdyb3RlOg0KPj4gDQo+Pj4gT24gT2N0IDI1 LCAyMDE2LCBhdCAxNDo0NSwgSi4gQnJ1Y2UgRmllbGRzIDxiZmllbGRzQGZpZWxkc2VzLm9yZz4g d3JvdGU6DQo+Pj4gDQo+Pj4gRnJvbTogIkouIEJydWNlIEZpZWxkcyIgPGJmaWVsZHNAcmVkaGF0 LmNvbT4NCj4+PiANCj4+PiBBcyBvZiBhYzRlOTdhYmNlOWIgInNjYXR0ZXJsaXN0OiBzZ19zZXRf YnVmKCkgYXJndW1lbnQgbXVzdCBiZSBpbiBsaW5lYXINCj4+PiBtYXBwaW5nIiwgc2dfc2V0X2J1 ZiBoaXRzIGEgQlVHIHdoZW4gbWFrZV9jaGVja3N1bV92Mi0+eGRyX3Byb2Nlc3NfYnVmLA0KPj4+ IGFtb25nIG90aGVyIGNhbGxlcnMsIHBhc3NlcyBpdCBtZW1vcnkgb24gdGhlIHN0YWNrLg0KPj4+ IA0KPj4+IFdlIG9ubHkgbmVlZCBhIHNjYXR0ZXJsaXN0IHRvIHBhc3MgdGhpcyB0byB0aGUgY3J5 cHRvIGNvZGUsIGFuZCBpdCBzZWVtcw0KPj4+IGxpa2Ugb3ZlcmtpbGwgdG8gcmVxdWlyZSBrbWFs bG9jJ2QgbWVtb3J5IGp1c3QgdG8gZW5jcnlwdCBhIGZldyBieXRlcywNCj4+PiBidXQgZm9yIG5v dyB0aGlzIHNlZW1zIHRoZSBiZXN0IGZpeC4NCj4+PiANCj4+PiBOb3RlIG1hbnkgb2YgdGhlc2Ug Y2FsbGVycyBhcmUgaW4gdGhlIE5GUyB3cml0ZSBwYXRocywgc28gd2Ugc2hvdWxkbid0DQo+Pj4g cmVhbGx5IGJlIGFsbG9jYXRpbmcgR0ZQX0tFUk5FTC4gIEJ1dCB3ZSBhbHJlYWR5IGhhdmUgb3Ro ZXIgYWxsb2NhdGlvbnMNCj4+PiBpbiB0aGVzZSBjb2RlIHBhdGhzLiAgQSBsYXJnZXIgcmVkZXNp Z24gbWF5IGJlIG5lY2Vzc2FyeSB0byBhbGxvdw0KPj4+IGFsbG9jYXRpb25zIHRvIGJlIGRvbmUg ZWFybGllci4NCj4+IA0KPj4gTkFDS+KApiAgSSBhZ3JlZSB0aGF0IHRoZXJlIG1heSBhbHJlYWR5 IGJlIGJvcmthZ2UgaW4gUlBDU0VDX0dTUy1sYW5kLCBidXQgdGhhdOKAmXMgbm90IGEgcmVhc29u IHRvIGJlIGFkZGluZyB0byB0aGUgcGlsZSBvZiB0aGluZ3MgdGhhdCBuZWVkIHRvIGJlIGZpeGVk 4oCmIFRoZSBhbGxvY2F0aW9ucyB0aGF0IGxpZSBpbiB0aGUgUlBDIGNsaWVudOKAmXMgZW5jb2Rl L2RlY29kZSBwYXRoIGRvIG5lZWQgdG8gYmUgR0ZQX05PRlPigKYuDQo+IA0KPiBPSy4gIEFueSBk aXNhZHZhbnRhZ2UgdG8ga2VlcGluZyB0aGlzIHBhdGNoIHdpdGgganVzdCBHRlBfTk9GUw0KPiBh bGxvY2F0aW9ucyBldmVyeXdoZXJlIHRoYXQgbWlnaHQgYmUgaW4gdGhlIHdyaXRlIHBhdGg/DQoN Ckl04oCZcyB3aGF0IHdlIGhhdmUgdG8gZG8gZXZlcnl3aGVyZSBlbHNlIGluIHRoZSBSUEMgY2xp ZW50LCBzbyBJIGNhbuKAmXQgc2VlIHdoeSBpdCBzaG91bGQgYmUgYSBwcm9ibGVtLg0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): DB39073B-8C4B-48BC-8D46-7B6173966037-7I+n7zu2hftEKMMhf/[email protected]
Quoted 10 lines · Trond Myklebust
Quoted 8 lines · Fields Bruce James
Quoted 1 line · Trond Myklebust
NACK… I agree that there may already be borkage in RPCSEC_GSS-land, but that’s not a reason to be adding to the pile of things that need to be fixed… The allocations that lie in the RPC client’s encode/decode path do need to be GFP_NOFS….
OK. Any disadvantage to keeping this patch with just
GFP_NOFSallocations everywhere that might be in the write path?It’s what we have to do everywhere else in the RPC client, so I can’t see why it should be a problem.
OK, the below is what I intend to merge if nobody spots another problem.
--b.
commit 2876a34466ce
Author: J. Bruce Fields <[email protected]> Date: Tue Oct 18 16:30:09 2016 -0400
sunrpc: don't pass on-stack memory to sg_set_buf
As of ac4e97abce9b "scatterlist: sg_set_buf() argument must be in linear mapping", sg_set_buf hits a BUG when make_checksum_v2->xdr_process_buf, among other callers, passes it memory on the stack.
We only need a scatterlist to pass this to the crypto code, and it seems like overkill to require kmalloc'd memory just to encrypt a few bytes, but for now this seems the best fix.
Many of these callers are in the NFS write paths, so we allocate with GFP_NOFS. It might be possible to do without allocations here entirely, but that would probably be a bigger project.
net/sunrpc/auth_gss/auth_gss.c +9 -4
@@ -1616,7 +1616,7 @@ gss_validate(struct rpc_task *task, __be32 *p) {struct rpc_cred *cred = task->tk_rqstp->rq_cred;struct gss_cl_ctx *ctx =gss_cred_get_ctx (cred); -__be32 seq; +__be32 *seq =NULL ;struct kvec iov;struct xdr_buf verf_buf;struct xdr_netobj mic;
@@ -1631,9 +1631,12 @@ gss_validate(struct rpc_task *task, __be32 *p) goto out_bad;if (flav !=RPC_AUTH_GSS ) goto out_bad; - seq =htonl (task->tk_rqstp->rq_seqno); - iov.iov_base = &seq; - iov.iov_len =sizeof (seq); + seq =kmalloc (4 ,GFP_NOFS ); +if (!seq) + goto out_bad; + *seq =htonl (task->tk_rqstp->rq_seqno); + iov.iov_base = seq; + iov.iov_len =4 ;xdr_buf_from_iov (&iov, &verf_buf); mic.data = (u8 *)p; mic.len = len;
@@ -1653,11 +1656,13 @@ gss_validate(struct rpc_task *task, __be32 *p)gss_put_ctx (ctx);dprintk ("RPC: %5u %s: gss_verify_mic succeeded.\n" , task->tk_pid, __func__); +kfree (seq);return p +XDR_QUADLEN (len); out_bad:gss_put_ctx (ctx);dprintk ("RPC: %5u %s failed ret %ld.\n" , task->tk_pid, __func__,PTR_ERR (ret)); +kfree (seq);return ret; }
net/sunrpc/auth_gss/gss_krb5_crypto.c +48 -34
179 lines not on this page · raw message
net/sunrpc/auth_gss/svcauth_gss.c +14 -7
@@ -718,30 +718,37 @@ gss_write_null_verf(struct svc_rqst *rqstp)static int gss_write_verf (struct svc_rqst *rqstp,struct gss_ctx *ctx_id,u32 seq) { -__be32 xdr_seq; +__be32 *xdr_seq;u32 maj_stat;struct xdr_buf verf_data;struct xdr_netobj mic;__be32 *p;struct kvec iov; +int err =-1 ;svc_putnl (rqstp->rq_res.head,RPC_AUTH_GSS ); - xdr_seq =htonl (seq); + xdr_seq =kmalloc (4 ,GFP_KERNEL ); +if (!xdr_seq) +return -1 ; + *xdr_seq =htonl (seq); - iov.iov_base = &xdr_seq; - iov.iov_len =sizeof (xdr_seq); + iov.iov_base = xdr_seq; + iov.iov_len =4 ;xdr_buf_from_iov (&iov, &verf_data); p = rqstp->rq_res.head->iov_base + rqstp->rq_res.head->iov_len; mic.data = (u8 *)(p +1 ); maj_stat =gss_get_mic (ctx_id, &verf_data, &mic);if (maj_stat !=GSS_S_COMPLETE ) -return -1 ; + goto out; *p++ =htonl (mic.len);memset ((u8 *)p + mic.len,0 ,round_up_to_quad (mic.len) - mic.len); p +=XDR_QUADLEN (mic.len);if (!xdr_ressize_check (rqstp, p)) -return -1 ; -return 0 ; + goto out; + err =0 ; +out: +kfree (xdr_seq); +return err; }struct gss_domain {
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · Fields Bruce James
Quoted 4 lines · Trond Myklebust
Quoted 4 lines · Fields Bruce James
On Tue, Oct 25, 2016 at 07:34:43PM +0000, Trond Myklebust wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
NACK… I agree that there may already be borkage in RPCSEC_GSS-land, but that’s not a reason to be adding to the pile of things that need to be fixed… The allocations that lie in the RPC client’s encode/decode path do need to be GFP_NOFS….
OK. Any disadvantage to keeping this patch with just
GFP_NOFSallocations everywhere that might be in the write path?It’s what we have to do everywhere else in the RPC client, so I can’t see why it should be a problem.
OK, the below is what I intend to merge if nobody spots another problem.
And we may as well fix up some more easy stuff while we're there.
And actually moving the other crypto_alloc_* calls may be pretty easy too, I'll see if I can get to it later....
--b.
commit 7e72f3a7c4db
Author: J. Bruce Fields <[email protected]> Date: Wed Oct 26 16:03:00 2016 -0400
sunrpc: GFP_KERNEL should be GFP_NOFS in crypto code
Writes may depend on the auth_gss crypto code, so we shouldn't be allocating with GFP_KERNEL there.
This still leaves some crypto_alloc_* calls which end up doing GFP_KERNEL allocations in the crypto code. Those could probably done at crypto import time.
net/sunrpc/auth_gss/gss_krb5_crypto.c +6 -6
@@ -200,7 +200,7 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,if (IS_ERR (hmac_md5)) goto out_free_md5; - req =ahash_request_alloc (md5,GFP_KERNEL ); + req =ahash_request_alloc (md5,GFP_NOFS );if (!req) goto out_free_hmac_md5;
@@ -230,7 +230,7 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen, goto out;ahash_request_free (req); - req =ahash_request_alloc (hmac_md5,GFP_KERNEL ); + req =ahash_request_alloc (hmac_md5,GFP_NOFS );if (!req) goto out_free_hmac_md5;
@@ -299,7 +299,7 @@ make_checksum(struct krb5_ctx *kctx, char *header, int hdrlen,if (IS_ERR (tfm)) goto out_free_cksum; - req =ahash_request_alloc (tfm,GFP_KERNEL ); + req =ahash_request_alloc (tfm,GFP_NOFS );if (!req) goto out_free_ahash;
@@ -397,7 +397,7 @@ make_checksum_v2(struct krb5_ctx *kctx, char *header, int hdrlen, goto out_free_cksum; checksumlen =crypto_ahash_digestsize (tfm); - req =ahash_request_alloc (tfm,GFP_KERNEL ); + req =ahash_request_alloc (tfm,GFP_NOFS );if (!req) goto out_free_ahash;
@@ -963,7 +963,7 @@ krb5_rc4_setup_seq_key(struct krb5_ctx *kctx, struct crypto_skcipher *cipher, } desc =kmalloc (sizeof (*desc) +crypto_shash_descsize (hmac), -GFP_KERNEL ); +GFP_NOFS );if (!desc) {dprintk ("%s: failed to allocate shash descriptor for '%s'\n" , __func__, kctx->gk5e->cksum_name);
@@ -1030,7 +1030,7 @@ krb5_rc4_setup_enc_key(struct krb5_ctx *kctx, struct crypto_skcipher *cipher, } desc =kmalloc (sizeof (*desc) +crypto_shash_descsize (hmac), -GFP_KERNEL ); +GFP_NOFS );if (!desc) {dprintk ("%s: failed to allocate shash descriptor for '%s'\n" , __func__, kctx->gk5e->cksum_name);
net/sunrpc/auth_gss/gss_krb5_mech.c +1 -2
@@ -451,8 +451,7 @@ context_derive_keys_rc4(struct krb5_ctx *ctx) goto out_err_free_hmac; - desc =kmalloc (sizeof (*desc) +crypto_shash_descsize (hmac), -GFP_KERNEL ); + desc =kmalloc (sizeof (*desc) +crypto_shash_descsize (hmac),GFP_NOFS );if (!desc) {dprintk ("%s: failed to allocate hash descriptor for '%s'\n" , __func__, ctx->gk5e->cksum_name);