Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: fix_priv_head
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200723193811.GG31487@fieldses.org>
Date:   Thu, 23 Jul 2020 16:23:05 -0400
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: 7bit
Message-Id: <94381D74-3563-4071-A0CF-4EC016744FEC@oracle.com>
References: <3799C9E0-DFF3-450C-A815-14BAFAC97EA8@oracle.com>
 <20200723193811.GG31487@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> 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.

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