| Message ID | 20240112084540.3729001-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] SUNRPC: fix a memleak in gss_import_v2_context | expand |
On Fri, 2024-01-12 at 16:45 +0800, Zhipeng Lu wrote: > The ctx->mech_used.data allocated by kmemdup is not freed in neither > gss_import_v2_context nor it only caller radeon_driver_open_kms. I'm not sure what this has to do with the radeon driver? The changelog probably needs to be revised. > Thus, this patch reform the last call of gss_import_v2_context to the > gss_krb5_import_ctx_v2, preventing the memleak while keepping the return > formation. > > Fixes: 47d848077629 ("gss_krb5: handle new context format from gssd") > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > --- > Changelog: > > v2: add non-error case > --- > net/sunrpc/auth_gss/gss_krb5_mech.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c > index e31cfdf7eadc..5e6f90d73858 100644 > --- a/net/sunrpc/auth_gss/gss_krb5_mech.c > +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c > @@ -398,6 +398,7 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx, > u64 seq_send64; > int keylen; > u32 time32; > + int ret; > > p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags)); > if (IS_ERR(p)) > @@ -450,8 +451,16 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx, > } > ctx->mech_used.len = gss_kerberos_mech.gm_oid.len; > > - return gss_krb5_import_ctx_v2(ctx, gfp_mask); > + ret = gss_krb5_import_ctx_v2(ctx, gfp_mask); > + if (ret) { > + p = ERR_PTR(ret); > + goto out_free; > + }; > > + return 0; > + > +out_free: > + kfree(ctx->mech_used.data); > out_err: > return PTR_ERR(p); > } Nice catch! Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, 12 Jan 2024 16:45:38 +0800 Zhipeng Lu wrote: > + if (ret) { > + p = ERR_PTR(ret); > + goto out_free; > + }; cocci says: net/sunrpc/auth_gss/gss_krb5_mech.c:458:2-3: Unneeded semicolon
> On Jan 12, 2024, at 2:24 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 12 Jan 2024 16:45:38 +0800 Zhipeng Lu wrote: >> + if (ret) { >> + p = ERR_PTR(ret); >> + goto out_free; >> + }; > > cocci says: > > net/sunrpc/auth_gss/gss_krb5_mech.c:458:2-3: Unneeded semicolon I planned to take this patch via NFSD's "for v6.9" branch. I can remove that semicolon. Thanks! -- Chuck Lever
On Fri, 12 Jan 2024 19:27:40 +0000 Chuck Lever III wrote: > > cocci says: > > > > net/sunrpc/auth_gss/gss_krb5_mech.c:458:2-3: Unneeded semicolon > > I planned to take this patch via NFSD's "for v6.9" branch. > I can remove that semicolon. Thanks! Sorry for the lack of clarity, I wasn't intending to take it. The patch did get into our checking machinery and the warning was reported, so I figured why not say so on the list. I'll mention the intentions more clearly next time!
On Fri, Jan 12, 2024 at 04:45:38PM +0800, Zhipeng Lu wrote: > The ctx->mech_used.data allocated by kmemdup is not freed in neither > gss_import_v2_context nor it only caller radeon_driver_open_kms. Should radeon_driver_open_kms be gss_krb5_import_sec_context? Also, perhaps it is useful to write something like this: ... gss_krb5_import_sec_context, which frees ctx on error. > Thus, this patch reform the last call of gss_import_v2_context to the > gss_krb5_import_ctx_v2, preventing the memleak while keepping the return > formation. > > Fixes: 47d848077629 ("gss_krb5: handle new context format from gssd") > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> Hi Zhipeng Lu, Other than the comment above, I agree with your analysis. And that although the problem has changed form slightly, it was originally introduced by the cited commit. I also agree that your fix. ...
> On Jan 15, 2024, at 6:09 AM, Simon Horman <horms@kernel.org> wrote: > > On Fri, Jan 12, 2024 at 04:45:38PM +0800, Zhipeng Lu wrote: >> The ctx->mech_used.data allocated by kmemdup is not freed in neither >> gss_import_v2_context nor it only caller radeon_driver_open_kms. > > Should radeon_driver_open_kms be gss_krb5_import_sec_context? > > Also, perhaps it is useful to write something like this: > > ... gss_krb5_import_sec_context, which frees ctx on error. If Zhipeng agrees to this suggestion, I can change the patch description in my tree. A v3 is not necessary. >> Thus, this patch reform the last call of gss_import_v2_context to the >> gss_krb5_import_ctx_v2, preventing the memleak while keepping the return >> formation. >> >> Fixes: 47d848077629 ("gss_krb5: handle new context format from gssd") >> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > > Hi Zhipeng Lu, > > Other than the comment above, I agree with your analysis. > And that although the problem has changed form slightly, > it was originally introduced by the cited commit. > I also agree that your fix. > > ... -- Chuck Lever
> > On Jan 15, 2024, at 6:09 AM, Simon Horman <horms@kernel.org> wrote: > > > > On Fri, Jan 12, 2024 at 04:45:38PM +0800, Zhipeng Lu wrote: > >> The ctx->mech_used.data allocated by kmemdup is not freed in neither > >> gss_import_v2_context nor it only caller radeon_driver_open_kms. > > > > Should radeon_driver_open_kms be gss_krb5_import_sec_context? > > > > Also, perhaps it is useful to write something like this: > > > > ... gss_krb5_import_sec_context, which frees ctx on error. Yes, you are right, I proberly mixed up it to another patch :(. And the first sentence of the patch description should be: The ctx->mech_used.data allocated by kmemdup is not freed in neither gss_import_v2_context nor it only caller gss_krb5_import_sec_context, which frees ctx on error. > > If Zhipeng agrees to this suggestion, I can change the > patch description in my tree. A v3 is not necessary. Yes, I agree with Simon's suggestion and I give the corrected description above. > > >> Thus, this patch reform the last call of gss_import_v2_context to the > >> gss_krb5_import_ctx_v2, preventing the memleak while keepping the return > >> formation. > >> > >> Fixes: 47d848077629 ("gss_krb5: handle new context format from gssd") > >> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > > > > Hi Zhipeng Lu, > > > > Other than the comment above, I agree with your analysis. > > And that although the problem has changed form slightly, > > it was originally introduced by the cited commit. > > I also agree that your fix. > > > > ... > > -- > Chuck Lever > >
diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c index e31cfdf7eadc..5e6f90d73858 100644 --- a/net/sunrpc/auth_gss/gss_krb5_mech.c +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c @@ -398,6 +398,7 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx, u64 seq_send64; int keylen; u32 time32; + int ret; p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags)); if (IS_ERR(p)) @@ -450,8 +451,16 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx, } ctx->mech_used.len = gss_kerberos_mech.gm_oid.len; - return gss_krb5_import_ctx_v2(ctx, gfp_mask); + ret = gss_krb5_import_ctx_v2(ctx, gfp_mask); + if (ret) { + p = ERR_PTR(ret); + goto out_free; + }; + return 0; + +out_free: + kfree(ctx->mech_used.data); out_err: return PTR_ERR(p); }
The ctx->mech_used.data allocated by kmemdup is not freed in neither gss_import_v2_context nor it only caller radeon_driver_open_kms. Thus, this patch reform the last call of gss_import_v2_context to the gss_krb5_import_ctx_v2, preventing the memleak while keepping the return formation. Fixes: 47d848077629 ("gss_krb5: handle new context format from gssd") Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> --- Changelog: v2: add non-error case --- net/sunrpc/auth_gss/gss_krb5_mech.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)