| Message ID | 20190201195814.11389.4023.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC GSS overhaul | expand |
On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote: > The key action of xdr_buf_trim() is that it shortens buf->len, the > length of the xdr_buf' content. The other actions -- shortening the > head, pages, and tail components -- are actually not necessary. In > some cases, changing the size of those components corrupts the RPC > message contained in the buffer. That's really burying the lede.... Is there an actual user-visible bug here? --b. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > include/linux/sunrpc/xdr.h | 1 - > net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++--- > net/sunrpc/auth_gss/svcauth_gss.c | 2 +- > net/sunrpc/xdr.c | 41 ----------------------------------- > 4 files changed, 6 insertions(+), 46 deletions(-) > > diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h > index 69161cb..4ae398c 100644 > --- a/include/linux/sunrpc/xdr.h > +++ b/include/linux/sunrpc/xdr.h > @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le > extern void xdr_shift_buf(struct xdr_buf *, size_t); > extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *); > extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int); > -extern void xdr_buf_trim(struct xdr_buf *, unsigned int); > extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int); > extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); > extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c > index 5cdde6c..14a0aff 100644 > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c > @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift) > */ > movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len); > movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip; > - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > > - buf->head[0].iov_len); > + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > > + buf->head[0].iov_len) > + return GSS_S_FAILURE; > memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen); > buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip; > buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip; > > /* Trim off the trailing "extra count" and checksum blob */ > - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip); > + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip; > + > return GSS_S_COMPLETE; > } > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c > index 152790e..f1aabab 100644 > --- a/net/sunrpc/auth_gss/svcauth_gss.c > +++ b/net/sunrpc/auth_gss/svcauth_gss.c > @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom) > if (svc_getnl(&buf->head[0]) != seq) > goto out; > /* trim off the mic and padding at the end before returning */ > - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4); > + buf->len -= 4 + round_up_to_quad(mic.len); > stat = 0; > out: > kfree(mic.data); > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > index 5f0aa53..4bce619 100644 > --- a/net/sunrpc/xdr.c > +++ b/net/sunrpc/xdr.c > @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len) > } > EXPORT_SYMBOL_GPL(xdr_buf_subsegment); > > -/** > - * xdr_buf_trim - lop at most "len" bytes off the end of "buf" > - * @buf: buf to be trimmed > - * @len: number of bytes to reduce "buf" by > - * > - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note > - * that it's possible that we'll trim less than that amount if the xdr_buf is > - * too small, or if (for instance) it's all in the head and the parser has > - * already read too far into it. > - */ > -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len) > -{ > - size_t cur; > - unsigned int trim = len; > - > - if (buf->tail[0].iov_len) { > - cur = min_t(size_t, buf->tail[0].iov_len, trim); > - buf->tail[0].iov_len -= cur; > - trim -= cur; > - if (!trim) > - goto fix_len; > - } > - > - if (buf->page_len) { > - cur = min_t(unsigned int, buf->page_len, trim); > - buf->page_len -= cur; > - trim -= cur; > - if (!trim) > - goto fix_len; > - } > - > - if (buf->head[0].iov_len) { > - cur = min_t(size_t, buf->head[0].iov_len, trim); > - buf->head[0].iov_len -= cur; > - trim -= cur; > - } > -fix_len: > - buf->len -= (len - trim); > -} > -EXPORT_SYMBOL_GPL(xdr_buf_trim); > - > static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len) > { > unsigned int this_len;
> On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote: > > On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote: >> The key action of xdr_buf_trim() is that it shortens buf->len, the >> length of the xdr_buf' content. The other actions -- shortening the >> head, pages, and tail components -- are actually not necessary. In >> some cases, changing the size of those components corrupts the RPC >> message contained in the buffer. > > That's really burying the lede.... Is there an actual user-visible bug > here? I don't think so. This is more of the form: a) the function does fundamentally the wrong thing, so b) certain changes to this code path result is unexpected and incorrect behavior Thus typically only developers hacking on this code run into a problem. > --b. > >> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> include/linux/sunrpc/xdr.h | 1 - >> net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++--- >> net/sunrpc/auth_gss/svcauth_gss.c | 2 +- >> net/sunrpc/xdr.c | 41 ----------------------------------- >> 4 files changed, 6 insertions(+), 46 deletions(-) >> >> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h >> index 69161cb..4ae398c 100644 >> --- a/include/linux/sunrpc/xdr.h >> +++ b/include/linux/sunrpc/xdr.h >> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le >> extern void xdr_shift_buf(struct xdr_buf *, size_t); >> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *); >> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int); >> -extern void xdr_buf_trim(struct xdr_buf *, unsigned int); >> extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int); >> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); >> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); >> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c >> index 5cdde6c..14a0aff 100644 >> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c >> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c >> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift) >> */ >> movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len); >> movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip; >> - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > >> - buf->head[0].iov_len); >> + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > >> + buf->head[0].iov_len) >> + return GSS_S_FAILURE; >> memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen); >> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip; >> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip; >> >> /* Trim off the trailing "extra count" and checksum blob */ >> - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip); >> + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip; >> + >> return GSS_S_COMPLETE; >> } >> >> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c >> index 152790e..f1aabab 100644 >> --- a/net/sunrpc/auth_gss/svcauth_gss.c >> +++ b/net/sunrpc/auth_gss/svcauth_gss.c >> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom) >> if (svc_getnl(&buf->head[0]) != seq) >> goto out; >> /* trim off the mic and padding at the end before returning */ >> - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4); >> + buf->len -= 4 + round_up_to_quad(mic.len); >> stat = 0; >> out: >> kfree(mic.data); >> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c >> index 5f0aa53..4bce619 100644 >> --- a/net/sunrpc/xdr.c >> +++ b/net/sunrpc/xdr.c >> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len) >> } >> EXPORT_SYMBOL_GPL(xdr_buf_subsegment); >> >> -/** >> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf" >> - * @buf: buf to be trimmed >> - * @len: number of bytes to reduce "buf" by >> - * >> - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note >> - * that it's possible that we'll trim less than that amount if the xdr_buf is >> - * too small, or if (for instance) it's all in the head and the parser has >> - * already read too far into it. >> - */ >> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len) >> -{ >> - size_t cur; >> - unsigned int trim = len; >> - >> - if (buf->tail[0].iov_len) { >> - cur = min_t(size_t, buf->tail[0].iov_len, trim); >> - buf->tail[0].iov_len -= cur; >> - trim -= cur; >> - if (!trim) >> - goto fix_len; >> - } >> - >> - if (buf->page_len) { >> - cur = min_t(unsigned int, buf->page_len, trim); >> - buf->page_len -= cur; >> - trim -= cur; >> - if (!trim) >> - goto fix_len; >> - } >> - >> - if (buf->head[0].iov_len) { >> - cur = min_t(size_t, buf->head[0].iov_len, trim); >> - buf->head[0].iov_len -= cur; >> - trim -= cur; >> - } >> -fix_len: >> - buf->len -= (len - trim); >> -} >> -EXPORT_SYMBOL_GPL(xdr_buf_trim); >> - >> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len) >> { >> unsigned int this_len; -- Chuck Lever
On Mon, Feb 04, 2019 at 02:49:11PM -0500, Chuck Lever wrote: > > > > On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote: > > > > On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote: > >> The key action of xdr_buf_trim() is that it shortens buf->len, the > >> length of the xdr_buf' content. The other actions -- shortening the > >> head, pages, and tail components -- are actually not necessary. In > >> some cases, changing the size of those components corrupts the RPC > >> message contained in the buffer. > > > > That's really burying the lede.... Is there an actual user-visible bug > > here? > > I don't think so. This is more of the form: > > a) the function does fundamentally the wrong thing, so > > b) certain changes to this code path result is unexpected and incorrect > behavior > > Thus typically only developers hacking on this code run into a problem. OK, got it. It'd help just to make it clear in the changelog that that this is an accident waiting to happen rather than a current bug (as far as we know). --b. > > > > --b. > > > >> > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > >> --- > >> include/linux/sunrpc/xdr.h | 1 - > >> net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++--- > >> net/sunrpc/auth_gss/svcauth_gss.c | 2 +- > >> net/sunrpc/xdr.c | 41 ----------------------------------- > >> 4 files changed, 6 insertions(+), 46 deletions(-) > >> > >> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h > >> index 69161cb..4ae398c 100644 > >> --- a/include/linux/sunrpc/xdr.h > >> +++ b/include/linux/sunrpc/xdr.h > >> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le > >> extern void xdr_shift_buf(struct xdr_buf *, size_t); > >> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *); > >> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int); > >> -extern void xdr_buf_trim(struct xdr_buf *, unsigned int); > >> extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int); > >> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); > >> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); > >> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c > >> index 5cdde6c..14a0aff 100644 > >> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c > >> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c > >> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift) > >> */ > >> movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len); > >> movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip; > >> - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > > >> - buf->head[0].iov_len); > >> + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > > >> + buf->head[0].iov_len) > >> + return GSS_S_FAILURE; > >> memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen); > >> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip; > >> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip; > >> > >> /* Trim off the trailing "extra count" and checksum blob */ > >> - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip); > >> + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip; > >> + > >> return GSS_S_COMPLETE; > >> } > >> > >> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c > >> index 152790e..f1aabab 100644 > >> --- a/net/sunrpc/auth_gss/svcauth_gss.c > >> +++ b/net/sunrpc/auth_gss/svcauth_gss.c > >> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom) > >> if (svc_getnl(&buf->head[0]) != seq) > >> goto out; > >> /* trim off the mic and padding at the end before returning */ > >> - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4); > >> + buf->len -= 4 + round_up_to_quad(mic.len); > >> stat = 0; > >> out: > >> kfree(mic.data); > >> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > >> index 5f0aa53..4bce619 100644 > >> --- a/net/sunrpc/xdr.c > >> +++ b/net/sunrpc/xdr.c > >> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len) > >> } > >> EXPORT_SYMBOL_GPL(xdr_buf_subsegment); > >> > >> -/** > >> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf" > >> - * @buf: buf to be trimmed > >> - * @len: number of bytes to reduce "buf" by > >> - * > >> - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note > >> - * that it's possible that we'll trim less than that amount if the xdr_buf is > >> - * too small, or if (for instance) it's all in the head and the parser has > >> - * already read too far into it. > >> - */ > >> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len) > >> -{ > >> - size_t cur; > >> - unsigned int trim = len; > >> - > >> - if (buf->tail[0].iov_len) { > >> - cur = min_t(size_t, buf->tail[0].iov_len, trim); > >> - buf->tail[0].iov_len -= cur; > >> - trim -= cur; > >> - if (!trim) > >> - goto fix_len; > >> - } > >> - > >> - if (buf->page_len) { > >> - cur = min_t(unsigned int, buf->page_len, trim); > >> - buf->page_len -= cur; > >> - trim -= cur; > >> - if (!trim) > >> - goto fix_len; > >> - } > >> - > >> - if (buf->head[0].iov_len) { > >> - cur = min_t(size_t, buf->head[0].iov_len, trim); > >> - buf->head[0].iov_len -= cur; > >> - trim -= cur; > >> - } > >> -fix_len: > >> - buf->len -= (len - trim); > >> -} > >> -EXPORT_SYMBOL_GPL(xdr_buf_trim); > >> - > >> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len) > >> { > >> unsigned int this_len; > > -- > Chuck Lever > >
> On Feb 4, 2019, at 3:00 PM, Bruce Fields <bfields@fieldses.org> wrote: > > On Mon, Feb 04, 2019 at 02:49:11PM -0500, Chuck Lever wrote: >> >> >>> On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote: >>> >>> On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote: >>>> The key action of xdr_buf_trim() is that it shortens buf->len, the >>>> length of the xdr_buf' content. The other actions -- shortening the >>>> head, pages, and tail components -- are actually not necessary. In >>>> some cases, changing the size of those components corrupts the RPC >>>> message contained in the buffer. >>> >>> That's really burying the lede.... Is there an actual user-visible bug >>> here? >> >> I don't think so. This is more of the form: >> >> a) the function does fundamentally the wrong thing, so >> >> b) certain changes to this code path result is unexpected and incorrect >> behavior >> >> Thus typically only developers hacking on this code run into a problem. > > OK, got it. It'd help just to make it clear in the changelog that that > this is an accident waiting to happen rather than a current bug (as far > as we know). With said improvement to the changelog, can I add your Acked-by when I submit this through Anna's tree? > --b. > >> >> >>> --b. >>> >>>> >>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >>>> --- >>>> include/linux/sunrpc/xdr.h | 1 - >>>> net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++--- >>>> net/sunrpc/auth_gss/svcauth_gss.c | 2 +- >>>> net/sunrpc/xdr.c | 41 ----------------------------------- >>>> 4 files changed, 6 insertions(+), 46 deletions(-) >>>> >>>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h >>>> index 69161cb..4ae398c 100644 >>>> --- a/include/linux/sunrpc/xdr.h >>>> +++ b/include/linux/sunrpc/xdr.h >>>> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le >>>> extern void xdr_shift_buf(struct xdr_buf *, size_t); >>>> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *); >>>> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int); >>>> -extern void xdr_buf_trim(struct xdr_buf *, unsigned int); >>>> extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int); >>>> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); >>>> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); >>>> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c >>>> index 5cdde6c..14a0aff 100644 >>>> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c >>>> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c >>>> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift) >>>> */ >>>> movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len); >>>> movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip; >>>> - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > >>>> - buf->head[0].iov_len); >>>> + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > >>>> + buf->head[0].iov_len) >>>> + return GSS_S_FAILURE; >>>> memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen); >>>> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip; >>>> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip; >>>> >>>> /* Trim off the trailing "extra count" and checksum blob */ >>>> - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip); >>>> + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip; >>>> + >>>> return GSS_S_COMPLETE; >>>> } >>>> >>>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c >>>> index 152790e..f1aabab 100644 >>>> --- a/net/sunrpc/auth_gss/svcauth_gss.c >>>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c >>>> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom) >>>> if (svc_getnl(&buf->head[0]) != seq) >>>> goto out; >>>> /* trim off the mic and padding at the end before returning */ >>>> - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4); >>>> + buf->len -= 4 + round_up_to_quad(mic.len); >>>> stat = 0; >>>> out: >>>> kfree(mic.data); >>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c >>>> index 5f0aa53..4bce619 100644 >>>> --- a/net/sunrpc/xdr.c >>>> +++ b/net/sunrpc/xdr.c >>>> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len) >>>> } >>>> EXPORT_SYMBOL_GPL(xdr_buf_subsegment); >>>> >>>> -/** >>>> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf" >>>> - * @buf: buf to be trimmed >>>> - * @len: number of bytes to reduce "buf" by >>>> - * >>>> - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note >>>> - * that it's possible that we'll trim less than that amount if the xdr_buf is >>>> - * too small, or if (for instance) it's all in the head and the parser has >>>> - * already read too far into it. >>>> - */ >>>> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len) >>>> -{ >>>> - size_t cur; >>>> - unsigned int trim = len; >>>> - >>>> - if (buf->tail[0].iov_len) { >>>> - cur = min_t(size_t, buf->tail[0].iov_len, trim); >>>> - buf->tail[0].iov_len -= cur; >>>> - trim -= cur; >>>> - if (!trim) >>>> - goto fix_len; >>>> - } >>>> - >>>> - if (buf->page_len) { >>>> - cur = min_t(unsigned int, buf->page_len, trim); >>>> - buf->page_len -= cur; >>>> - trim -= cur; >>>> - if (!trim) >>>> - goto fix_len; >>>> - } >>>> - >>>> - if (buf->head[0].iov_len) { >>>> - cur = min_t(size_t, buf->head[0].iov_len, trim); >>>> - buf->head[0].iov_len -= cur; >>>> - trim -= cur; >>>> - } >>>> -fix_len: >>>> - buf->len -= (len - trim); >>>> -} >>>> -EXPORT_SYMBOL_GPL(xdr_buf_trim); >>>> - >>>> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len) >>>> { >>>> unsigned int this_len; >> >> -- >> Chuck Lever -- Chuck Lever
On Mon, Feb 04, 2019 at 03:07:26PM -0500, Chuck Lever wrote: > > > > On Feb 4, 2019, at 3:00 PM, Bruce Fields <bfields@fieldses.org> wrote: > > > > On Mon, Feb 04, 2019 at 02:49:11PM -0500, Chuck Lever wrote: > >> > >> > >>> On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote: > >>> > >>> On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote: > >>>> The key action of xdr_buf_trim() is that it shortens buf->len, the > >>>> length of the xdr_buf' content. The other actions -- shortening the > >>>> head, pages, and tail components -- are actually not necessary. In > >>>> some cases, changing the size of those components corrupts the RPC > >>>> message contained in the buffer. > >>> > >>> That's really burying the lede.... Is there an actual user-visible bug > >>> here? > >> > >> I don't think so. This is more of the form: > >> > >> a) the function does fundamentally the wrong thing, so > >> > >> b) certain changes to this code path result is unexpected and incorrect > >> behavior > >> > >> Thus typically only developers hacking on this code run into a problem. > > > > OK, got it. It'd help just to make it clear in the changelog that that > > this is an accident waiting to happen rather than a current bug (as far > > as we know). > > With said improvement to the changelog, can I add your Acked-by > when I submit this through Anna's tree? Sure, thanks. --b.
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h index 69161cb..4ae398c 100644 --- a/include/linux/sunrpc/xdr.h +++ b/include/linux/sunrpc/xdr.h @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le extern void xdr_shift_buf(struct xdr_buf *, size_t); extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *); extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int); -extern void xdr_buf_trim(struct xdr_buf *, unsigned int); extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int); extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int); diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c index 5cdde6c..14a0aff 100644 --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift) */ movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len); movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip; - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > - buf->head[0].iov_len); + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen > + buf->head[0].iov_len) + return GSS_S_FAILURE; memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen); buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip; buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip; /* Trim off the trailing "extra count" and checksum blob */ - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip); + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip; + return GSS_S_COMPLETE; } diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c index 152790e..f1aabab 100644 --- a/net/sunrpc/auth_gss/svcauth_gss.c +++ b/net/sunrpc/auth_gss/svcauth_gss.c @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom) if (svc_getnl(&buf->head[0]) != seq) goto out; /* trim off the mic and padding at the end before returning */ - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4); + buf->len -= 4 + round_up_to_quad(mic.len); stat = 0; out: kfree(mic.data); diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c index 5f0aa53..4bce619 100644 --- a/net/sunrpc/xdr.c +++ b/net/sunrpc/xdr.c @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len) } EXPORT_SYMBOL_GPL(xdr_buf_subsegment); -/** - * xdr_buf_trim - lop at most "len" bytes off the end of "buf" - * @buf: buf to be trimmed - * @len: number of bytes to reduce "buf" by - * - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note - * that it's possible that we'll trim less than that amount if the xdr_buf is - * too small, or if (for instance) it's all in the head and the parser has - * already read too far into it. - */ -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len) -{ - size_t cur; - unsigned int trim = len; - - if (buf->tail[0].iov_len) { - cur = min_t(size_t, buf->tail[0].iov_len, trim); - buf->tail[0].iov_len -= cur; - trim -= cur; - if (!trim) - goto fix_len; - } - - if (buf->page_len) { - cur = min_t(unsigned int, buf->page_len, trim); - buf->page_len -= cur; - trim -= cur; - if (!trim) - goto fix_len; - } - - if (buf->head[0].iov_len) { - cur = min_t(size_t, buf->head[0].iov_len, trim); - buf->head[0].iov_len -= cur; - trim -= cur; - } -fix_len: - buf->len -= (len - trim); -} -EXPORT_SYMBOL_GPL(xdr_buf_trim); - static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len) { unsigned int this_len;
The key action of xdr_buf_trim() is that it shortens buf->len, the length of the xdr_buf' content. The other actions -- shortening the head, pages, and tail components -- are actually not necessary. In some cases, changing the size of those components corrupts the RPC message contained in the buffer. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- include/linux/sunrpc/xdr.h | 1 - net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++--- net/sunrpc/auth_gss/svcauth_gss.c | 2 +- net/sunrpc/xdr.c | 41 ----------------------------------- 4 files changed, 6 insertions(+), 46 deletions(-)