On 03/15/2010 11:58 AM, Trond Myklebust wrote:
> On Mon, 2010-03-15 at 08:20 -0400, steved@xxxxxxxxxx wrote:
>> From: Kevin Coffman <kwc@xxxxxxxxxxxxxx>
>>
>> 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 <kwc@xxxxxxxxxxxxxx>
>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx>
>> ---
>> 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 <linux/sunrpc/gss_err.h>
>> #include <linux/sunrpc/gss_asn1.h>
>>
>> +/* 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,
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
