Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753084AbdHGKnM (ORCPT ); Mon, 7 Aug 2017 06:43:12 -0400 Received: from mail-wr0-f195.google.com ([209.85.128.195]:34190 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752893AbdHGKnL (ORCPT ); Mon, 7 Aug 2017 06:43:11 -0400 From: Cihangir Akturk To: lustre-devel@lists.lustre.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Cc: oleg.drokin@intel.com, andreas.dilger@intel.com, jsimmons@infradead.org, gregkh@linuxfoundation.org, Cihangir Akturk Subject: [PATCH] staging: lustre: mgc: fix potential use after free in error path Date: Mon, 7 Aug 2017 13:43:00 +0300 Message-Id: <1502102580-29556-1-git-send-email-cakturk@gmail.com> X-Mailer: git-send-email 2.7.4 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3011 Lines: 87 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 --- 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