* [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf
@ 2016-10-25 18:45 J. Bruce Fields
2016-10-25 19:34 ` Trond Myklebust
0 siblings, 1 reply; 6+ messages in thread
From: J. Bruce Fields @ 2016-10-25 18:45 UTC (permalink / raw)
To: linux-nfs; +Cc: Rusty Russell, Christoph Hellwig, linux-crypto
From: "J. Bruce Fields" <bfields@redhat.com>
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.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
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(-)
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index d8bd97a5a7c9..6ecc30058f95 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -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;
}
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 244245bcbbd2..8a280a810439 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -166,8 +166,8 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
unsigned int usage, struct xdr_netobj *cksumout)
{
struct scatterlist sg[1];
- int err;
- u8 checksumdata[GSS_KRB5_MAX_CKSUM_LEN];
+ int err = -1;
+ u8 *checksumdata;
u8 rc4salt[4];
struct crypto_ahash *md5;
struct crypto_ahash *hmac_md5;
@@ -187,23 +187,22 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
return GSS_S_FAILURE;
}
+ checksumdata = kmalloc(GSS_KRB5_MAX_CKSUM_LEN, GFP_KERNEL);
+ if (!checksumdata)
+ return GSS_S_FAILURE;
+
md5 = crypto_alloc_ahash("md5", 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(md5))
- return GSS_S_FAILURE;
+ goto out_free_cksum;
hmac_md5 = crypto_alloc_ahash(kctx->gk5e->cksum_name, 0,
CRYPTO_ALG_ASYNC);
- if (IS_ERR(hmac_md5)) {
- crypto_free_ahash(md5);
- return GSS_S_FAILURE;
- }
+ if (IS_ERR(hmac_md5))
+ goto out_free_md5;
req = ahash_request_alloc(md5, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(hmac_md5);
- crypto_free_ahash(md5);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_hmac_md5;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -232,11 +231,8 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
ahash_request_free(req);
req = ahash_request_alloc(hmac_md5, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(hmac_md5);
- crypto_free_ahash(md5);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_hmac_md5;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -258,8 +254,12 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
cksumout->len = kctx->gk5e->cksumlength;
out:
ahash_request_free(req);
- crypto_free_ahash(md5);
+out_free_hmac_md5:
crypto_free_ahash(hmac_md5);
+out_free_md5:
+ crypto_free_ahash(md5);
+out_free_cksum:
+ kfree(checksumdata);
return err ? GSS_S_FAILURE : 0;
}
@@ -276,8 +276,8 @@ make_checksum(struct krb5_ctx *kctx, char *header, int hdrlen,
struct crypto_ahash *tfm;
struct ahash_request *req;
struct scatterlist sg[1];
- int err;
- u8 checksumdata[GSS_KRB5_MAX_CKSUM_LEN];
+ int err = -1;
+ u8 *checksumdata;
unsigned int checksumlen;
if (kctx->gk5e->ctype == CKSUMTYPE_HMAC_MD5_ARCFOUR)
@@ -291,15 +291,17 @@ make_checksum(struct krb5_ctx *kctx, char *header, int hdrlen,
return GSS_S_FAILURE;
}
+ checksumdata = kmalloc(GSS_KRB5_MAX_CKSUM_LEN, GFP_KERNEL);
+ if (checksumdata == NULL)
+ return GSS_S_FAILURE;
+
tfm = crypto_alloc_ahash(kctx->gk5e->cksum_name, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
- return GSS_S_FAILURE;
+ goto out_free_cksum;
req = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(tfm);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_ahash;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -349,7 +351,10 @@ make_checksum(struct krb5_ctx *kctx, char *header, int hdrlen,
cksumout->len = kctx->gk5e->cksumlength;
out:
ahash_request_free(req);
+out_free_ahash:
crypto_free_ahash(tfm);
+out_free_cksum:
+ kfree(checksumdata);
return err ? GSS_S_FAILURE : 0;
}
@@ -368,8 +373,8 @@ make_checksum_v2(struct krb5_ctx *kctx, char *header, int hdrlen,
struct crypto_ahash *tfm;
struct ahash_request *req;
struct scatterlist sg[1];
- int err;
- u8 checksumdata[GSS_KRB5_MAX_CKSUM_LEN];
+ int err = -1;
+ u8 *checksumdata;
unsigned int checksumlen;
if (kctx->gk5e->keyed_cksum == 0) {
@@ -383,16 +388,18 @@ make_checksum_v2(struct krb5_ctx *kctx, char *header, int hdrlen,
return GSS_S_FAILURE;
}
+ checksumdata = kmalloc(GSS_KRB5_MAX_CKSUM_LEN, GFP_KERNEL);
+ if (!checksumdata)
+ return GSS_S_FAILURE;
+
tfm = crypto_alloc_ahash(kctx->gk5e->cksum_name, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
- return GSS_S_FAILURE;
+ goto out_free_cksum;
checksumlen = crypto_ahash_digestsize(tfm);
req = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(tfm);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_ahash;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -433,7 +440,10 @@ make_checksum_v2(struct krb5_ctx *kctx, char *header, int hdrlen,
}
out:
ahash_request_free(req);
+out_free_ahash:
crypto_free_ahash(tfm);
+out_free_cksum:
+ kfree(checksumdata);
return err ? GSS_S_FAILURE : 0;
}
@@ -666,14 +676,17 @@ gss_krb5_cts_crypt(struct crypto_skcipher *cipher, struct xdr_buf *buf,
u32 ret;
struct scatterlist sg[1];
SKCIPHER_REQUEST_ON_STACK(req, cipher);
- u8 data[GSS_KRB5_MAX_BLOCKSIZE * 2];
+ u8 *data;
struct page **save_pages;
u32 len = buf->len - offset;
- if (len > ARRAY_SIZE(data)) {
+ if (len > GSS_KRB5_MAX_BLOCKSIZE * 2) {
WARN_ON(0);
return -ENOMEM;
}
+ data = kmalloc(GSS_KRB5_MAX_BLOCKSIZE * 2, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
/*
* For encryption, we want to read from the cleartext
@@ -708,6 +721,7 @@ gss_krb5_cts_crypt(struct crypto_skcipher *cipher, struct xdr_buf *buf,
ret = write_bytes_to_xdr_buf(buf, offset, data, len);
out:
+ kfree(data);
return ret;
}
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index d67f7e1bc82d..45662d7f0943 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -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 {
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf
2016-10-25 18:45 [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf J. Bruce Fields
@ 2016-10-25 19:34 ` Trond Myklebust
2016-10-25 19:59 ` Fields Bruce James
0 siblings, 1 reply; 6+ messages in thread
From: Trond Myklebust @ 2016-10-25 19:34 UTC (permalink / raw)
To: Fields Bruce James
Cc: List Linux NFS Mailing, Rusty Russell, Christoph Hellwig,
linux-crypto@vger.kernel.org
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=
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf
2016-10-25 19:34 ` Trond Myklebust
@ 2016-10-25 19:59 ` Fields Bruce James
2016-10-25 21:47 ` Trond Myklebust
0 siblings, 1 reply; 6+ messages in thread
From: Fields Bruce James @ 2016-10-25 19:59 UTC (permalink / raw)
To: Trond Myklebust
Cc: List Linux NFS Mailing, Rusty Russell, Christoph Hellwig,
linux-crypto@vger.kernel.org
On Tue, Oct 25, 2016 at 07:34:43PM +0000, Trond Myklebust wrote:
>
> > On Oct 25, 2016, at 14:45, J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > From: "J. Bruce Fields" <bfields@redhat.com>
> >
> > 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.
>
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf
2016-10-25 19:59 ` Fields Bruce James
@ 2016-10-25 21:47 ` Trond Myklebust
2016-10-28 13:20 ` Fields Bruce James
0 siblings, 1 reply; 6+ messages in thread
From: Trond Myklebust @ 2016-10-25 21:47 UTC (permalink / raw)
To: Fields Bruce James
Cc: List Linux NFS Mailing, Rusty Russell, Christoph Hellwig,
linux-crypto@vger.kernel.org
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=
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf
2016-10-25 21:47 ` Trond Myklebust
@ 2016-10-28 13:20 ` Fields Bruce James
2016-10-28 13:22 ` Fields Bruce James
0 siblings, 1 reply; 6+ messages in thread
From: Fields Bruce James @ 2016-10-28 13:20 UTC (permalink / raw)
To: Trond Myklebust
Cc: List Linux NFS Mailing, Rusty Russell, Christoph Hellwig,
linux-crypto@vger.kernel.org
On Tue, Oct 25, 2016 at 09:47:16PM +0000, Trond Myklebust wrote:
>
> > On Oct 25, 2016, at 15:59, Fields Bruce James <bfields@fieldses.org> wrote:
> >
> > On Tue, Oct 25, 2016 at 07:34:43PM +0000, Trond Myklebust wrote:
> >> 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?
>
> 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 <bfields@redhat.com>
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.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index d8bd97a5a7c9..3dfd769dc5b5 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -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;
}
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 244245bcbbd2..90115ceefd49 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -166,8 +166,8 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
unsigned int usage, struct xdr_netobj *cksumout)
{
struct scatterlist sg[1];
- int err;
- u8 checksumdata[GSS_KRB5_MAX_CKSUM_LEN];
+ int err = -1;
+ u8 *checksumdata;
u8 rc4salt[4];
struct crypto_ahash *md5;
struct crypto_ahash *hmac_md5;
@@ -187,23 +187,22 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
return GSS_S_FAILURE;
}
+ checksumdata = kmalloc(GSS_KRB5_MAX_CKSUM_LEN, GFP_NOFS);
+ if (!checksumdata)
+ return GSS_S_FAILURE;
+
md5 = crypto_alloc_ahash("md5", 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(md5))
- return GSS_S_FAILURE;
+ goto out_free_cksum;
hmac_md5 = crypto_alloc_ahash(kctx->gk5e->cksum_name, 0,
CRYPTO_ALG_ASYNC);
- if (IS_ERR(hmac_md5)) {
- crypto_free_ahash(md5);
- return GSS_S_FAILURE;
- }
+ if (IS_ERR(hmac_md5))
+ goto out_free_md5;
req = ahash_request_alloc(md5, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(hmac_md5);
- crypto_free_ahash(md5);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_hmac_md5;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -232,11 +231,8 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
ahash_request_free(req);
req = ahash_request_alloc(hmac_md5, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(hmac_md5);
- crypto_free_ahash(md5);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_hmac_md5;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -258,8 +254,12 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
cksumout->len = kctx->gk5e->cksumlength;
out:
ahash_request_free(req);
- crypto_free_ahash(md5);
+out_free_hmac_md5:
crypto_free_ahash(hmac_md5);
+out_free_md5:
+ crypto_free_ahash(md5);
+out_free_cksum:
+ kfree(checksumdata);
return err ? GSS_S_FAILURE : 0;
}
@@ -276,8 +276,8 @@ make_checksum(struct krb5_ctx *kctx, char *header, int hdrlen,
struct crypto_ahash *tfm;
struct ahash_request *req;
struct scatterlist sg[1];
- int err;
- u8 checksumdata[GSS_KRB5_MAX_CKSUM_LEN];
+ int err = -1;
+ u8 *checksumdata;
unsigned int checksumlen;
if (kctx->gk5e->ctype == CKSUMTYPE_HMAC_MD5_ARCFOUR)
@@ -291,15 +291,17 @@ make_checksum(struct krb5_ctx *kctx, char *header, int hdrlen,
return GSS_S_FAILURE;
}
+ checksumdata = kmalloc(GSS_KRB5_MAX_CKSUM_LEN, GFP_NOFS);
+ if (checksumdata == NULL)
+ return GSS_S_FAILURE;
+
tfm = crypto_alloc_ahash(kctx->gk5e->cksum_name, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
- return GSS_S_FAILURE;
+ goto out_free_cksum;
req = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(tfm);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_ahash;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -349,7 +351,10 @@ make_checksum(struct krb5_ctx *kctx, char *header, int hdrlen,
cksumout->len = kctx->gk5e->cksumlength;
out:
ahash_request_free(req);
+out_free_ahash:
crypto_free_ahash(tfm);
+out_free_cksum:
+ kfree(checksumdata);
return err ? GSS_S_FAILURE : 0;
}
@@ -368,8 +373,8 @@ make_checksum_v2(struct krb5_ctx *kctx, char *header, int hdrlen,
struct crypto_ahash *tfm;
struct ahash_request *req;
struct scatterlist sg[1];
- int err;
- u8 checksumdata[GSS_KRB5_MAX_CKSUM_LEN];
+ int err = -1;
+ u8 *checksumdata;
unsigned int checksumlen;
if (kctx->gk5e->keyed_cksum == 0) {
@@ -383,16 +388,18 @@ make_checksum_v2(struct krb5_ctx *kctx, char *header, int hdrlen,
return GSS_S_FAILURE;
}
+ checksumdata = kmalloc(GSS_KRB5_MAX_CKSUM_LEN, GFP_NOFS);
+ if (!checksumdata)
+ return GSS_S_FAILURE;
+
tfm = crypto_alloc_ahash(kctx->gk5e->cksum_name, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
- return GSS_S_FAILURE;
+ goto out_free_cksum;
checksumlen = crypto_ahash_digestsize(tfm);
req = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req) {
- crypto_free_ahash(tfm);
- return GSS_S_FAILURE;
- }
+ if (!req)
+ goto out_free_ahash;
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
@@ -433,7 +440,10 @@ make_checksum_v2(struct krb5_ctx *kctx, char *header, int hdrlen,
}
out:
ahash_request_free(req);
+out_free_ahash:
crypto_free_ahash(tfm);
+out_free_cksum:
+ kfree(checksumdata);
return err ? GSS_S_FAILURE : 0;
}
@@ -666,14 +676,17 @@ gss_krb5_cts_crypt(struct crypto_skcipher *cipher, struct xdr_buf *buf,
u32 ret;
struct scatterlist sg[1];
SKCIPHER_REQUEST_ON_STACK(req, cipher);
- u8 data[GSS_KRB5_MAX_BLOCKSIZE * 2];
+ u8 *data;
struct page **save_pages;
u32 len = buf->len - offset;
- if (len > ARRAY_SIZE(data)) {
+ if (len > GSS_KRB5_MAX_BLOCKSIZE * 2) {
WARN_ON(0);
return -ENOMEM;
}
+ data = kmalloc(GSS_KRB5_MAX_BLOCKSIZE * 2, GFP_NOFS);
+ if (!data)
+ return -ENOMEM;
/*
* For encryption, we want to read from the cleartext
@@ -708,6 +721,7 @@ gss_krb5_cts_crypt(struct crypto_skcipher *cipher, struct xdr_buf *buf,
ret = write_bytes_to_xdr_buf(buf, offset, data, len);
out:
+ kfree(data);
return ret;
}
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index d67f7e1bc82d..45662d7f0943 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -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 {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf
2016-10-28 13:20 ` Fields Bruce James
@ 2016-10-28 13:22 ` Fields Bruce James
0 siblings, 0 replies; 6+ messages in thread
From: Fields Bruce James @ 2016-10-28 13:22 UTC (permalink / raw)
To: Trond Myklebust
Cc: List Linux NFS Mailing, Rusty Russell, Christoph Hellwig,
linux-crypto@vger.kernel.org
On Fri, Oct 28, 2016 at 09:20:08AM -0400, Fields Bruce James wrote:
> On Tue, Oct 25, 2016 at 09:47:16PM +0000, Trond Myklebust wrote:
> >
> > > On Oct 25, 2016, at 15:59, Fields Bruce James <bfields@fieldses.org> wrote:
> > >
> > > On Tue, Oct 25, 2016 at 07:34:43PM +0000, Trond Myklebust wrote:
> > >> 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?
> >
> > 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 <bfields@redhat.com>
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.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 90115ceefd49..fb39284ec174 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -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);
diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
index 60595835317a..7bb2514aadd9 100644
--- a/net/sunrpc/auth_gss/gss_krb5_mech.c
+++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
@@ -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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-28 13:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-25 18:45 [PATCH] sunrpc: don't pass on-stack memory to sg_set_buf J. Bruce Fields
2016-10-25 19:34 ` Trond Myklebust
2016-10-25 19:59 ` Fields Bruce James
2016-10-25 21:47 ` Trond Myklebust
2016-10-28 13:20 ` Fields Bruce James
2016-10-28 13:22 ` Fields Bruce James
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).