> On Jul 23, 2020, at 3:38 PM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> On Thu, Jul 23, 2020 at 01:46:19PM -0400, Chuck Lever wrote:
>> Hi Bruce-
>>
>> I'm trying to figure out if fix_priv_head is still necessary. This
>> was introduced by 7c9fdcfb1b64 ("[PATCH] knfsd: svcrpc: gss:
>> server-side implementation of rpcsec_gss privacy").
>>
>> static void
>> fix_priv_head(struct xdr_buf *buf, int pad)
>> {
>> if (buf->page_len == 0) {
>> /* We need to adjust head and buf->len in tandem in this
>> * case to make svc_defer() work--it finds the original
>> * buffer start using buf->len - buf->head[0].iov_len. */
>> buf->head[0].iov_len -= pad;
>> }
>> }
>>
>> It doesn't seem like unwrapping can ever result in a buffer length that
>> is not quad-aligned. Is that simply a characteristic of modern enctypes?
And: how is it correct to subtract "pad" ? if the length of the content
is not aligned, this truncates it. Instead, shouldn't the length be
extended to the next quad-boundary?
> This code is before any unwrapping. We're looking at the length of the
> encrypted (wrapped) object here, not the unwrapped buffer.
fix_priv_head() is called twice: once before and once after gss_unwrap.
There is also this adjustment, just after the gss_unwrap() call:
maj_stat = gss_unwrap(ctx, 0, priv_len, buf);
pad = priv_len - buf->len;
buf->len -= pad;
This is actually a bug, now that gss_unwrap adjusts buf->len: subtracting
"pad" can make buf->len go negative. I'd like to remove this code, but
I'd first like to understand how it will effect the code that follows
immediately after:
offset = xdr_pad_size(buf->head[0].iov_len);
if (offset) {
buf->buflen = RPCSVC_MAXPAYLOAD;
xdr_shift_buf(buf, offset);
fix_priv_head(buf, pad);
}
> When using privacy, the body of an rpcsec_gss request is a single opaque
> object consisting of the wrapped data. So the question is whether
> there's any case where the length of that object can be less than the
> length remaining in the received buffer.
>
> I think the only reason for bytes at the end is, yes, that that opaque
> object is not a multiple of 4 and so rpc requires padding at the end.
Newer enctypes seem to put something substantial beyond the end of
the opaque. That's why gss_unwrap_kerberos_v2() finishes with a
call to xdr_buf_trim().
But I'm not sure why the receiver should care about a misaligned size
of the opaque.
The GSS mechanism's unwrap method should set buf->len to the size
of the unencrypted payload message, and for RPC, that size should
always be a multiple of four, and will exclude any of those extra
bytes.
--
Chuck Lever
