> On Jul 23, 2020, at 9:17 PM, Bruce Fields <bfields@xxxxxxxxxxxx> 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@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.
>
> 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@xxxxxxxxxxxxxxxxxxxxx/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
