From: Steve Dickson Subject: Re: [PATCH 01/22] gss_krb5: introduce encryption type framework Date: Tue, 16 Mar 2010 16:49:36 -0400 Message-ID: <4B9FEEE0.8040306@RedHat.com> References: <1268655627-18712-1-git-send-email-steved@redhat.com> <1268655627-18712-2-git-send-email-steved@redhat.com> <1268668733.2993.90.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: linux-nfs@vger.kernel.org To: Trond Myklebust Return-path: Received: from mx1.redhat.com ([209.132.183.28]:30266 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751881Ab0CPUuV (ORCPT ); Tue, 16 Mar 2010 16:50:21 -0400 In-Reply-To: <1268668733.2993.90.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: On 03/15/2010 11:58 AM, Trond Myklebust wrote: > On Mon, 2010-03-15 at 08:20 -0400, steved@redhat.com wrote: >> From: Kevin Coffman >> >> Make the client and server code consistent regarding the extra buffer >> space made available for the auth code when wrapping data. >> >> Add some comments/documentation about the available buffer space >> in the xdr_buf head and tail when gss_wrap is called. >> >> Add a compile-time check to make sure we are not exceeding the available >> buffer space. >> >> Add a central function to shift head data. >> >> Signed-off-by: Kevin Coffman >> Signed-off-by: Steve Dickson >> --- >> include/linux/sunrpc/gss_krb5.h | 25 ++++++++++++++ >> net/sunrpc/auth_gss/auth_gss.c | 14 ++++++-- >> net/sunrpc/auth_gss/gss_krb5_crypto.c | 56 +++++++++++++++++++++++++++++++++ >> net/sunrpc/auth_gss/gss_krb5_wrap.c | 7 ++-- >> net/sunrpc/auth_gss/gss_mech_switch.c | 14 ++++++++ >> net/sunrpc/auth_gss/svcauth_gss.c | 15 +++++++++ >> 6 files changed, 123 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/sunrpc/gss_krb5.h b/include/linux/sunrpc/gss_krb5.h >> index e7bbdba..5bb227e 100644 >> --- a/include/linux/sunrpc/gss_krb5.h >> +++ b/include/linux/sunrpc/gss_krb5.h >> @@ -40,6 +40,12 @@ >> #include >> #include >> >> +/* Maximum checksum function output for the supported crypto algorithms */ >> +#define GSS_KRB5_MAX_CKSUM_LEN (20) >> + >> +/* Maximum blocksize for the supported crypto algorithms */ >> +#define GSS_KRB5_MAX_BLOCKSIZE (16) >> + >> struct krb5_ctx { >> int initiate; /* 1 = initiating, 0 = accepting */ >> struct crypto_blkcipher *enc; >> @@ -113,6 +119,22 @@ enum seal_alg { >> #define ENCTYPE_DES3_CBC_SHA1 0x0010 >> #define ENCTYPE_UNKNOWN 0x01ff >> >> +/* >> + * This compile-time check verifies that we will not exceed the >> + * slack space allotted by the client and server auth_gss code >> + * before they call gss_wrap(). >> + */ >> +#define GSS_KRB5_SLACK_CHECK \ >> + BUILD_BUG_ON(GSS_KRB5_TOK_HDR_LEN /* gss token header */ \ >> + + GSS_KRB5_MAX_CKSUM_LEN /* gss token checksum */ \ >> + + GSS_KRB5_MAX_BLOCKSIZE /* confounder */ \ >> + + GSS_KRB5_MAX_BLOCKSIZE /* possible padding */ \ >> + + GSS_KRB5_TOK_HDR_LEN /* encrypted hdr in v2 token */\ >> + + GSS_KRB5_MAX_CKSUM_LEN /* encryption hmac */ \ >> + + 4 + 4 /* RPC verifier */ \ >> + + GSS_KRB5_TOK_HDR_LEN \ >> + + GSS_KRB5_MAX_CKSUM_LEN > RPC_MAX_AUTH_SIZE) >> + >> s32 >> make_checksum(char *, char *header, int hdrlen, struct xdr_buf *body, >> int body_offset, struct xdr_netobj *cksum); >> @@ -157,3 +179,6 @@ s32 >> krb5_get_seq_num(struct crypto_blkcipher *key, >> unsigned char *cksum, >> unsigned char *buf, int *direction, u32 *seqnum); >> + >> +int >> +shift_head_data(struct xdr_buf *buf, unsigned int base, unsigned int shiftlen); >> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c >> index 0cfccc2..a268368 100644 >> --- a/net/sunrpc/auth_gss/auth_gss.c >> +++ b/net/sunrpc/auth_gss/auth_gss.c >> @@ -61,7 +61,7 @@ static const struct rpc_credops gss_nullops; >> # define RPCDBG_FACILITY RPCDBG_AUTH >> #endif >> >> -#define GSS_CRED_SLACK 1024 >> +#define GSS_CRED_SLACK (RPC_MAX_AUTH_SIZE * 2) >> /* length of a krb5 verifier (48), plus data added before arguments when >> * using integrity (two 4-byte integers): */ >> #define GSS_VERF_SLACK 100 >> @@ -1317,15 +1317,21 @@ gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx, >> inpages = snd_buf->pages + first; >> snd_buf->pages = rqstp->rq_enc_pages; >> snd_buf->page_base -= first << PAGE_CACHE_SHIFT; >> - /* Give the tail its own page, in case we need extra space in the >> - * head when wrapping: */ >> + /* >> + * Give the tail its own page, in case we need extra space in the >> + * head when wrapping: >> + * >> + * call_allocate() allocates twice the slack space required >> + * by the authentication flavor to rq_callsize. >> + * For GSS, slack is GSS_CRED_SLACK. >> + */ > > I'm all for improving the comments in the code, but could we please make > that a separate patch. Really? I'm curious... what is not clear about the above diff > >> if (snd_buf->page_len || snd_buf->tail[0].iov_len) { >> tmp = page_address(rqstp->rq_enc_pages[rqstp->rq_enc_pages_num - 1]); >> memcpy(tmp, snd_buf->tail[0].iov_base, snd_buf->tail[0].iov_len); >> snd_buf->tail[0].iov_base = tmp; >> } >> maj_stat = gss_wrap(ctx->gc_gss_ctx, offset, snd_buf, inpages); >> - /* RPC_SLACK_SPACE should prevent this ever happening: */ >> + /* slack space should prevent this ever happening: */ >> BUG_ON(snd_buf->len > snd_buf->buflen); >> status = -EIO; >> /* We're assuming that when GSS_S_CONTEXT_EXPIRED, the encryption was >> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c >> index c93fca2..d0f3371 100644 >> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c >> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c >> @@ -326,3 +326,59 @@ gss_decrypt_xdr_buf(struct crypto_blkcipher *tfm, struct xdr_buf *buf, >> >> return xdr_process_buf(buf, offset, buf->len - offset, decryptor, &desc); >> } >> + >> +/* >> + * This function makes the assumption that it was ultimately called >> + * from gss_wrap(). >> + * >> + * The client auth_gss code moves any existing tail data into a >> + * separate page before calling gss_wrap. >> + * The server svcauth_gss code ensures that both the head and the >> + * tail have slack space of RPC_MAX_AUTH_SIZE before calling gss_wrap. >> + * >> + * Even with that guarantee, this function may be called more than >> + * once in the processing of gss_wrap(). The best we can do is >> + * verify at compile-time (see GSS_KRB5_SLACK_CHECK) that the >> + * largest expected shift will fit within RPC_MAX_AUTH_SIZE. >> + * At run-time we can verify that a single invocation of this >> + * function doesn't attempt to use more the RPC_MAX_AUTH_SIZE. >> + */ >> + >> +int >> +shift_head_data(struct xdr_buf *buf, unsigned int base, unsigned int shiftlen) > ^^^^^^^^^^^^^^^ > This needs a better name in order to avoid polluting the global > namespace. xdr_extend_head(), perhaps? xdr_extend_head() it is... > >> +{ >> + u8 *p; >> + >> + if (shiftlen == 0) >> + return 0; >> + >> + GSS_KRB5_SLACK_CHECK; > ^^^^^^^^^^^^^^^^^^^^^ > Why is this a macro? To hide the ugliness of the BUILD_BUG_ON() macro which I think is a good thing... I would rather see that one line verse the 10 or so lines it hiding... > >> + BUG_ON(shiftlen > RPC_MAX_AUTH_SIZE); >> + >> + /* >> + * If there is a tail, and it shares a page with the head, >> + * make sure we don't clobber the tail. This is a just a >> + * defensive check. >> + */ >> + if (buf->tail[0].iov_base != NULL) { >> + if ((((long)buf->tail[0].iov_base >> PAGE_CACHE_SHIFT) == >> + ((long)buf->head[0].iov_base >> PAGE_CACHE_SHIFT)) && > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This is just too ugly to live, and is wrong to boot. If > buf->tail[0].iov_len == 0, then buf->tail[0].iov_base isn't even > defined... I'm just going to remove this check... > >> + buf->tail[0].iov_base - buf->head[0].iov_base < shiftlen) { >> + dprintk("%s: collision: head %p:%zu, tail %p:%zu, " >> + "shiftlen %u\n", >> + __func__, buf->head[0].iov_base, >> + buf->head[0].iov_len, buf->tail[0].iov_base, >> + buf->tail[0].iov_len, shiftlen); >> + return 1; >> + } >> + } >> + >> + p = buf->head[0].iov_base + base; >> + >> + memmove(p + shiftlen, p, buf->head[0].iov_len - base); >> + >> + buf->head[0].iov_len += shiftlen; >> + buf->len += shiftlen; >> + >> + return 0; >> +} >> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c >> index ae8e69b..a0660f5 100644 >> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c >> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c >> @@ -144,6 +144,7 @@ gss_wrap_kerberos(struct gss_ctx *ctx, int offset, >> >> dprintk("RPC: gss_wrap_kerberos\n"); >> >> + GSS_KRB5_SLACK_CHECK; > > ....and why do we have a second one here? Since this is a BUILD_BUG, > then surely we can check this just once. right... this one is gone... > >> now = get_seconds(); >> >> blocksize = crypto_blkcipher_blocksize(kctx->enc); >> @@ -156,11 +157,9 @@ gss_wrap_kerberos(struct gss_ctx *ctx, int offset, >> >> ptr = buf->head[0].iov_base + offset; >> /* shift data to make room for header. */ >> + shift_head_data(buf, offset, headlen); >> + >> /* XXX Would be cleverer to encrypt while copying. */ >> - /* XXX bounds checking, slack, etc. */ >> - memmove(ptr + headlen, ptr, buf->head[0].iov_len - offset); >> - buf->head[0].iov_len += headlen; >> - buf->len += headlen; >> BUG_ON((buf->len - offset - headlen) % blocksize); >> >> g_make_token_header(&kctx->mech_used, > >