fix_priv_head

fix_priv_head

[Chuck Lever]

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?

--
Chuck Lever

Re: fix_priv_head

[Bruce Fields]

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?

This code is beofre any unwrapping.  We're looking at the length of the
encrypted (wrapped) object here, not the unwrapped buffer.

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.

I think that might be possible with encryption types that use cipher
text stealing, but I'm not certain.

--b.

Re: fix_priv_head

[Chuck Lever]

> 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

Re: fix_priv_head

[Bruce Fields]

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?)

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

Honestly, I wrote this code 10 or 15 years ago and haven't thought hard
about it in about that long.  And at the time I didn't feel like I
understood it as well as I should either, there was too much fixing
things up to make them work and not enough work organizing it correctly.
I'm sure there must be a way to make it simpler....

--b.

Re: fix_priv_head

[Chuck Lever]

> 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

Re: fix_priv_head

[Bruce Fields]

On Fri, Jul 24, 2020 at 10:10:08AM -0400, Chuck Lever wrote:
> >> 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.

Thinking about it more, even if there was some gss mechanism returning
misaligned data, the best place to fix that would likely be in the
mechanism-specific code (partly for reasons noted in the comment right
here--it'll be more efficient to put the data in the right spot as you
encrypt it.)

--b.

Re: fix_priv_head

[Chuck Lever]

> On Jul 24, 2020, at 4:39 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Fri, Jul 24, 2020 at 10:10:08AM -0400, Chuck Lever wrote:
>>>> 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.
> 
> Thinking about it more, even if there was some gss mechanism returning
> misaligned data, the best place to fix that would likely be in the
> mechanism-specific code (partly for reasons noted in the comment right
> here--it'll be more efficient to put the data in the right spot as you
> encrypt it.)

In principal, I totally agree that the GSS mechanism's unwrap method is
the obvious place to handle mis-alignment. The practical challenge in
this code path is that the needs of the client and server receive logic
diverge just enough to make it annoying.


So another remark about this:

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

The comment complains about svc_defer, and that particular calculation
is still in that code. It seems like it would be better if a pointer
into buf->head was saved somewhere instead of trying to manufacture
it based on buf->len, which seems to be pretty unreliable.

If svc_defer was changed that way, that might help us get rid of at
least the first fix_priv_head call site.

--
Chuck Lever