[PATCH] staging: lustre: mgc: fix potential use after free in error path

[PATCH] staging: lustre: mgc: fix potential use after free in error path

[Cihangir Akturk]

The config_log_add() function first calls config_log_put() with the
variable 'cld' and then jumps to label 'out_cld', which will call
the same function with the same 'cld' variable. However, at this
point, 'cld' might have been already freed by the first invocation
of config_log_put(). Even if we remove the invocation at that point,
we will still get into trouble. This is because, in the error path,
just below the label 'out_cld', we try to put 'params_cls' and
'sptlrpc_cld', which might also have been freed by config_log_put().

The point is that, config_llog_data::cld_sptlrpc and
config_llog_data::cld_params members are assigned at the beginning
of this function.

To avoid this, do not call config_log_put() inside the else block,
immediately jump to 'out_cld' instead. Moreover, remove assignments
to config_llog_data::cld_sptlrpc and config_llog_data::cld_params at
the beginning, since we already assign them below in the function
with 'cld_lock' held.

As an additional benefit, code size gets smaller.

before:
text    data     bss     dec     hex filename
26188   2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o

after:
text    data     bss     dec     hex filename
26092   2256    4208   32556    7f2c drivers/staging/lustre/lustre/mgc/mgc_request.o

Signed-off-by: Cihangir Akturk <cakturk@gmail.com>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index eee0b66..36c3049 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
 	struct config_llog_data *cld;
 	struct config_llog_data *sptlrpc_cld;
 	struct config_llog_data *params_cld;
-	bool locked = false;
+	struct config_llog_data *recover_cld = NULL;
 	char			seclogname[32];
 	char			*ptr;
 	int			rc;
@@ -333,20 +333,14 @@ config_log_add(struct obd_device *obd, char *logname,
 		goto out_params;
 	}
 
-	cld->cld_sptlrpc = sptlrpc_cld;
-	cld->cld_params = params_cld;
-
 	LASSERT(lsi->lsi_lmd);
 	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
-		struct config_llog_data *recover_cld;
-
 		ptr = strrchr(seclogname, '-');
 		if (ptr) {
 			*ptr = 0;
 		} else {
 			CERROR("%s: sptlrpc log name not correct, %s: rc = %d\n",
 			       obd->obd_name, seclogname, -EINVAL);
-			config_log_put(cld);
 			rc = -EINVAL;
 			goto out_cld;
 		}
@@ -355,14 +349,10 @@ config_log_add(struct obd_device *obd, char *logname,
 			rc = PTR_ERR(recover_cld);
 			goto out_cld;
 		}
-
-		mutex_lock(&cld->cld_lock);
-		locked = true;
-		cld->cld_recover = recover_cld;
 	}
 
-	if (!locked)
-		mutex_lock(&cld->cld_lock);
+	mutex_lock(&cld->cld_lock);
+	cld->cld_recover = recover_cld;
 	cld->cld_params = params_cld;
 	cld->cld_sptlrpc = sptlrpc_cld;
 	mutex_unlock(&cld->cld_lock);
-- 
2.7.4

Re: [PATCH] staging: lustre: mgc: fix potential use after free in error path

[James Simmons]

On Mon, 7 Aug 2017, Cihangir Akturk wrote:

> The config_log_add() function first calls config_log_put() with the
> variable 'cld' and then jumps to label 'out_cld', which will call
> the same function with the same 'cld' variable. However, at this
> point, 'cld' might have been already freed by the first invocation
> of config_log_put(). Even if we remove the invocation at that point,
> we will still get into trouble. This is because, in the error path,
> just below the label 'out_cld', we try to put 'params_cls' and
> 'sptlrpc_cld', which might also have been freed by config_log_put().
> 
> The point is that, config_llog_data::cld_sptlrpc and
> config_llog_data::cld_params members are assigned at the beginning
> of this function.
> 
> To avoid this, do not call config_log_put() inside the else block,
> immediately jump to 'out_cld' instead. Moreover, remove assignments
> to config_llog_data::cld_sptlrpc and config_llog_data::cld_params at
> the beginning, since we already assign them below in the function
> with 'cld_lock' held.
> 
> As an additional benefit, code size gets smaller.
> 
> before:
> text    data     bss     dec     hex filename
> 26188   2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o
> 
> after:
> text    data     bss     dec     hex filename
> 26092   2256    4208   32556    7f2c drivers/staging/lustre/lustre/mgc/mgc_request.o
> 
> Signed-off-by: Cihangir Akturk <cakturk@gmail.com>
> ---
>  drivers/staging/lustre/lustre/mgc/mgc_request.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
> index eee0b66..36c3049 100644
> --- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
> +++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
> @@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
>  	struct config_llog_data *cld;
>  	struct config_llog_data *sptlrpc_cld;
>  	struct config_llog_data *params_cld;
> -	bool locked = false;
> +	struct config_llog_data *recover_cld = NULL;
>  	char			seclogname[32];
>  	char			*ptr;
>  	int			rc;
> @@ -333,20 +333,14 @@ config_log_add(struct obd_device *obd, char *logname,
>  		goto out_params;
>  	}
>  
> -	cld->cld_sptlrpc = sptlrpc_cld;
> -	cld->cld_params = params_cld;
> -
>  	LASSERT(lsi->lsi_lmd);
>  	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
> -		struct config_llog_data *recover_cld;
> -
>  		ptr = strrchr(seclogname, '-');
>  		if (ptr) {
>  			*ptr = 0;
>  		} else {
>  			CERROR("%s: sptlrpc log name not correct, %s: rc = %d\n",
>  			       obd->obd_name, seclogname, -EINVAL);
> -			config_log_put(cld);
>  			rc = -EINVAL;
>  			goto out_cld;
>  		}
> @@ -355,14 +349,10 @@ config_log_add(struct obd_device *obd, char *logname,
>  			rc = PTR_ERR(recover_cld);
>  			goto out_cld;
>  		}
> -
> -		mutex_lock(&cld->cld_lock);
> -		locked = true;
> -		cld->cld_recover = recover_cld;
>  	}
>  
> -	if (!locked)
> -		mutex_lock(&cld->cld_lock);
> +	mutex_lock(&cld->cld_lock);
> +	cld->cld_recover = recover_cld;
>  	cld->cld_params = params_cld;
>  	cld->cld_sptlrpc = sptlrpc_cld;
>  	mutex_unlock(&cld->cld_lock);
> -- 
> 2.7.4

Reviewed-by: James Simmons <jsimmons@infradead.org>