| Message ID | 20240523084716.524-1-chenhx.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | SUNRPC: return proper error from gss_wrap_req_priv | expand |
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 <chenhx.fnst@fujitsu.com> Fixes: 0c77668ddb4e ("SUNRPC: Introduce trace points in rpc_auth_gss.ko") Reviewed-by: Benjamin Coddington <bcodding@redhat.com> 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
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 <chenhx.fnst@fujitsu.com> > > Fixes: 0c77668ddb4e ("SUNRPC: Introduce trace points in rpc_auth_gss.ko") > Reviewed-by: Benjamin Coddington <bcodding@redhat.com> > > 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 <chuck.lever@oracle.com> > 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.
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)
don't return 0 if snd_buf->len really greater than snd_buf->buflen Signed-off-by: Chen Hanxiao <chenhx.fnst@fujitsu.com> --- net/sunrpc/auth_gss/auth_gss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)