2024-05-23 09:07:39

by Chen Hanxiao

Subject: [PATCH] SUNRPC: return proper error from gss_wrap_req_priv

don't return 0 if snd_buf->len really greater than snd_buf->buflen

Signed-off-by: Chen Hanxiao <[email protected]>
---
net/sunrpc/auth_gss/auth_gss.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index c7af0220f82f..369310909fc9 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1875,8 +1875,10 @@ gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
maj_stat = gss_wrap(ctx->gc_gss_ctx, offset, snd_buf, inpages);
/* slack space should prevent this ever happening: */
- if (unlikely(snd_buf->len > snd_buf->buflen))
+ if (unlikely(snd_buf->len > snd_buf->buflen)) {
+ status = -EIO;
goto wrap_failed;
+ }
/* We're assuming that when GSS_S_CONTEXT_EXPIRED, the encryption was
* done anyway, so it's safe to put the request on the wire: */
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
--
2.39.1

2024-05-30 13:52:40

by Benjamin Coddington

Subject: Re: [PATCH] SUNRPC: return proper error from gss_wrap_req_priv

On 23 May 2024, at 4:47, Chen Hanxiao wrote:

> don't return 0 if snd_buf->len really greater than snd_buf->buflen
>
> Signed-off-by: Chen Hanxiao <[email protected]>

Fixes: 0c77668ddb4e ("SUNRPC: Introduce trace points in rpc_auth_gss.ko")
Reviewed-by: Benjamin Coddington <[email protected]>

more below ..


> ---
> net/sunrpc/auth_gss/auth_gss.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index c7af0220f82f..369310909fc9 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -1875,8 +1875,10 @@ gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
> offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
> maj_stat = gss_wrap(ctx->gc_gss_ctx, offset, snd_buf, inpages);
> /* slack space should prevent this ever happening: */
> - if (unlikely(snd_buf->len > snd_buf->buflen))
> + if (unlikely(snd_buf->len > snd_buf->buflen)) {
> + status = -EIO;
> goto wrap_failed;

Maybe Chuck intended to jump to bad_wrap in 0c77668ddb4e? Interesting that
you found this considering "slack space should prevent this ever happening".

Ben

2024-05-30 15:26:03

by Chuck Lever III

Subject: Re: [PATCH] SUNRPC: return proper error from gss_wrap_req_priv

On Thu, May 30, 2024 at 09:51:02AM -0400, Benjamin Coddington wrote:
> On 23 May 2024, at 4:47, Chen Hanxiao wrote:
>
> > don't return 0 if snd_buf->len really greater than snd_buf->buflen
> >
> > Signed-off-by: Chen Hanxiao <[email protected]>
>
> Fixes: 0c77668ddb4e ("SUNRPC: Introduce trace points in rpc_auth_gss.ko")
> Reviewed-by: Benjamin Coddington <[email protected]>
>
> more below ..
>
>
> > ---
> > net/sunrpc/auth_gss/auth_gss.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > index c7af0220f82f..369310909fc9 100644
> > --- a/net/sunrpc/auth_gss/auth_gss.c
> > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > @@ -1875,8 +1875,10 @@ gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
> > offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
> > maj_stat = gss_wrap(ctx->gc_gss_ctx, offset, snd_buf, inpages);
> > /* slack space should prevent this ever happening: */
> > - if (unlikely(snd_buf->len > snd_buf->buflen))
> > + if (unlikely(snd_buf->len > snd_buf->buflen)) {
> > + status = -EIO;
> > goto wrap_failed;
>
> Maybe Chuck intended to jump to bad_wrap in 0c77668ddb4e?

bad_wrap is specifically for reporting a GSS failure, so "goto
wrap_failed;" is correct.

The bug here is that the earlier alloc_enc_pages() call clobbers the
default value of @status.

Reviewed-by: Chuck Lever <[email protected]>


> Interesting that
> you found this considering "slack space should prevent this ever happening".

That suggests that the slack space computation is somehow wrong,
which might be possible for one of the new enctypes..? Further
investigation is warranted.


--
Chuck Lever