Re: fix_priv_head

[Chuck Lever <chuck.lever@oracle.com>]

> On Jul 23, 2020, at 9:17 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Thu, Jul 23, 2020 at 04:23:05PM -0400, Chuck Lever wrote:
>> 
>> 
>>> On Jul 23, 2020, at 3:38 PM, Bruce Fields <bfields@fieldses.org> 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.
> 
> OK, sorry, I missed that.
> 
>> 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.
> 
> OK.  Looking at the code now....  I'm not sure I follow it, but I'll
> believe you.
> 
> (But if we've been leaving buf->len too short, why hasn't that been
> causing really obvious test failures?)

Probably it's because the server's receive paths don't rely on buf->len
because they traditionally use svc_getnl() and friends, which change
the size of the head buffer but never update buf->len.

Shortening goes unnoticed until gss_unwrap sets buf->len to a value
that is 32 or more bytes smaller than priv_len. When an RPC message
is smaller than that difference, then "buf->len -= pad;" results
in an underflow.

A more accurate but dangerously short buf->len is the result of

https://lore.kernel.org/linux-nfs/159554608522.6546.6837849890434723341.stgit@klimt.1015granger.net/T/#u

So, perhaps those two patches should be combined, since the first one
breaks the server.


>> 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);
>>        }

So if one of those patches removes "pad = priv_len - buf->len;"
then the above code will break.

But I'm trying to see when it is possible for gss_unwrap to
return a head buffer that is not quad-aligned. Not coming up
with any such scenario.


--
Chuck Lever