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