From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753655AbcHTJZX (ORCPT ); Sat, 20 Aug 2016 05:25:23 -0400 Received: from mx01-fr.bfs.de ([193.174.231.67]:38384 "EHLO mx01-fr.bfs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751350AbcHTJZV (ORCPT ); Sat, 20 Aug 2016 05:25:21 -0400 Message-ID: <57B821FB.8050404@bfs.de> Date: Sat, 20 Aug 2016 11:25:15 +0200 From: walter harms Reply-To: wharms@bfs.de User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.1.16) Gecko/20101125 SUSE/3.0.11 Thunderbird/3.0.11 MIME-Version: 1.0 CC: linux-mmc@vger.kernel.org, Adrian Hunter , Grant Grundler , Jens Axboe , Jon Hunter , Mike Christie , Shawn Lin , Ulf Hansson , LKML , kernel-janitors@vger.kernel.org, Julia Lawall Subject: Re: [PATCH 1/2] mmc-block: Use memdup_user() rather than duplicating its implementation References: <566ABCD9.1060404@users.sourceforge.net> <68870228-c6a0-5de9-daee-2522a1303857@users.sourceforge.net> <10db7d07-0cee-041f-8755-de412a12bc81@users.sourceforge.net> In-Reply-To: <10db7d07-0cee-041f-8755-de412a12bc81@users.sourceforge.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit To: unlisted-recipients:; (no To-header on input) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am 19.08.2016 23:10, schrieb SF Markus Elfring: > From: Markus Elfring > Date: Fri, 19 Aug 2016 22:46:38 +0200 > > * Reuse existing functionality from memdup_user() instead of keeping > duplicate source code. > > This issue was detected by using the Coccinelle software. > > * Delete the integer variable "err" then because the pointer > variable "idata" should be sufficient to handle return values alone > in this function. > > Signed-off-by: Markus Elfring > --- > drivers/mmc/card/block.c | 26 +++++++++----------------- > 1 file changed, 9 insertions(+), 17 deletions(-) > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > index 48a5dd7..6ce9492 100644 > --- a/drivers/mmc/card/block.c > +++ b/drivers/mmc/card/block.c > @@ -337,22 +337,21 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( > struct mmc_ioc_cmd __user *user) > { > struct mmc_blk_ioc_data *idata; > - int err; > > idata = kmalloc(sizeof(*idata), GFP_KERNEL); > if (!idata) { > - err = -ENOMEM; > + idata = ERR_PTR(-ENOMEM); > goto out; > } > > if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) { > - err = -EFAULT; > + idata = ERR_PTR(-EFAULT); > goto idata_err; > } > > idata->buf_bytes = (u64) idata->ic.blksz * idata->ic.blocks; > if (idata->buf_bytes > MMC_IOC_MAX_BYTES) { > - err = -EOVERFLOW; > + idata = ERR_PTR(-EOVERFLOW); > goto idata_err; > } > > @@ -361,26 +360,19 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( > return idata; > } > > - idata->buf = kmalloc(idata->buf_bytes, GFP_KERNEL); > - if (!idata->buf) { > - err = -ENOMEM; > + idata->buf = memdup_user((void __user *)(unsigned long) > + idata->ic.data_ptr, > + idata->buf_bytes); > + if (IS_ERR(idata->buf)) { > + idata = (void *) idata->buf; > goto idata_err; > } > - > - if (copy_from_user(idata->buf, (void __user *)(unsigned long) > - idata->ic.data_ptr, idata->buf_bytes)) { > - err = -EFAULT; > - goto copy_err; > - } > - > return idata; > > -copy_err: > - kfree(idata->buf); > idata_err: > kfree(idata); > out: > - return ERR_PTR(err); > + return idata; > } This looks strange, returning a freed pointer is a bad idea. I suggest a idata=NULL after kfree(). re, wh > > static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,