Date:   Fri, 10 Dec 2021 13:04:50 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Cc:     Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
        gregkh@linuxfoundation.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, Abaci Robot <abaci@linux.alibaba.com>
Subject: Re: [PATCH v2] staging: r8188eu: Use strndup_user instead of
 kmalloc/copy_from_user
Message-ID: <20211210100450.GN1956@kadam>
References: <20211210093701.55559-1-jiapeng.chong@linux.alibaba.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211210093701.55559-1-jiapeng.chong@linux.alibaba.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?jdoOnqJ0RUlWDHWxvnrY/3zHVCbQPGrqDSsrwF36lDxZpiUmEwrEUCxLuDgT?=
 =?us-ascii?Q?j1dLbT6hFQ0S5AMacoLWZUqBz+wKj3SqCBFAvK5Bw+QjAq7lsHCEdyKMXui3?=
 =?us-ascii?Q?OYpYyh98B5guSMTN8WPJsiRzljqMbE82Nx8Dw9OUiYO1eWmmmW2La/1OUdoD?=
 =?us-ascii?Q?AhfwDiaGwR+7Uxnb+LtPUsXPYkMr72oBAodaHSe4V2FbIR4VnTOsyEj7F9aE?=
 =?us-ascii?Q?UZKkehTuHFRpxbyFrEQr2OtgTP1Y3Qc0MFyiqb4MrM5RTfmJ299kPLzJcbWD?=
 =?us-ascii?Q?z1S+1azknoM0JHUYNqhXbsFFY2GtnwDG0MNvhduS66+rGNinmZ7uquAFUo6V?=
 =?us-ascii?Q?HJdLhU15C5GpFg33HpFMsH0oizhnCAuTK4Xsn728Xx/o7ygeC/x5DpxJ/7bE?=
 =?us-ascii?Q?k71oHbqzaypbwn3EDBZ1lO26u/5WWN1gQe943kjUlCchKgkGk9qG9zaY2/iw?=
 =?us-ascii?Q?rFUyoQtvDrYSP5ZxG4BVSXUkDjGObT5kTLUZyZ3vWIE4PimYBSCL7ZF5X4Cm?=
 =?us-ascii?Q?tfMn4D8Fwcddue1k06+IWG1dFO/GOgNWSVBppblbOlWyd/z/HF/M/TdRfQY0?=
 =?us-ascii?Q?4paXmtVFlff5nlWGyUben7o0ehHRTET/mEu+SfVk18YiGdaaq3gTbVH26MlR?=
 =?us-ascii?Q?89AjQ1wg0CnervsEA8iCh+m6T7yfgQXRhB1eNCSebxDgbJyKfXpV7rDq1JJv?=
 =?us-ascii?Q?fzbiSeO8/UPE8a7VkIw70nxXXEFR2e7IXHyIfLg/GI71fun/1vGsF+nvQ+yC?=
 =?us-ascii?Q?f047abP3my5Tuv2w5d1yDff9/NE3IoEM3nBwu/HIFUkbPx1jflesFwjRPRBq?=
 =?us-ascii?Q?xVubNFlcdNhz33Hla3HxIf82zLsTisisc51DMScgidCRGRM75GI4mBebw141?=
 =?us-ascii?Q?hG/rag/d5WQeQX27FQFLQb4WLDgMkFUoe2J64Cn3OMspvvkggmwW2sWXZemn?=
 =?us-ascii?Q?+f039D4WumUPiY29BW+CyEdWgUKOkr4/qlLa4wClaSBcR+algSbxjR0H6786?=
 =?us-ascii?Q?MeOgZS8IaCYTzPdqKa8ece6xxvCrh9u3w+Sqpak7SLfOnyCHykfpofEdizK5?=
 =?us-ascii?Q?LMUuQk+huV+ekasqnJdYk/1K2ZmXGID8UOqo9nLCd/6EBkiMfuyFjze+I8In?=
 =?us-ascii?Q?G6VXyoNgkDTTt6Fi+0K0RLumSrTugZn8rrETeKQve5t8qVksyVUeJoq8cHLa?=
 =?us-ascii?Q?vLA4AsCvpggeYikGlwxEvNPrTIfIIWKCnjxXc3cr1A3p5xBIIcs1X3uj+3vE?=
 =?us-ascii?Q?h9C6BsVfsuepfvSJMDjJoMASOid185vt1glRfscPccJDwdI9CbPS7P0R2Peq?=
 =?us-ascii?Q?wG+X9Ds83rUM8FcJg7y5zkCKiM7VfQOOZ/ycr7YqVLgGLIkr9CNajy6Y0XDg?=
 =?us-ascii?Q?ZLdhqlTbN7QHDhzCjwlQmrZBRtHa7RVbV6TsbM8GICzs5VQdeGT0XXlBPbWL?=
 =?us-ascii?Q?7DUdNnmpQZX5j76HtNFPl4RAwsKxYx4jsoJ4lYkRKiomdQ2sDLvoFAEildNI?=
 =?us-ascii?Q?sB3dofJ3n0tgrTHq1sgkQeghNtmIzLmQsnu32SfHzTJh/Z1CpMQvHOtMyCIV?=
 =?us-ascii?Q?XgJj+X4GLqXVNa5wdDetAWqTfZfTuXZUhCu/FGFwt3wltXmGIQ9kNiWW2YzG?=
 =?us-ascii?Q?BK4xNV2O5QJcyeuH728EJKsSDuny1D1CFBNBLltROUQq4vMujWWx9+P/er9K?=
 =?us-ascii?Q?LOTdxIpanNCAg7VehVEXxe1FObU=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0b956862-2327-4036-305f-08d9bbc48e1a
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Dec 2021 10:05:12.6648
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xIwHNDg8ldzYBiHJrGQJKKsiyzM2C6ChhIA2F3oBpZ0qONw1XvH/gsAk0miKmK9avwI4XAJw20aF9AUj4/F5tsnSy26OuYfMAFDh7qXy6Lg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1293
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10193 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 spamscore=0
 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112100055
X-Proofpoint-ORIG-GUID: Q4NaNED73RimwaGWEOGOThaMbbfO8If_
X-Proofpoint-GUID: Q4NaNED73RimwaGWEOGOThaMbbfO8If_
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Dec 10, 2021 at 05:37:01PM +0800, Jiapeng Chong wrote:
> This code assumes that the user is going to give us a NULL terminated

I wouldn't comment on this except that the Fixes tag is wrong so it
needs to be resent anyway.  NULL is a pointer.  When it's a character
'\0'; then it's just one L "NUL terminated".

> string which is not necessarily true.  The original code was buggy too.
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Leave this out.

> 
> Fix following coccicheck warning:
> 
> ./drivers/staging/r8188eu/os_dep/ioctl_linux.c:4253:8-15: WARNING
> opportunity for memdup_user.
> 
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Fixes: 7bdedfef085b ("staging: r8188eu: Remove mp, a.k.a. manufacturing process, code")

The fixes tag should be:

Fixes: 2b42bd58b321 ("staging: r8188eu: introduce new os_dep dir for RTL8188eu driver")


> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> ---
> Changes in v2:
>   -For the follow advice:https://lore.kernel.org/lkml/20211210083010.GM1956@kadam/

Don't put a link in the email.  Just say what the difference is.  No
one wants to have to click on a link.

> 
>  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> index 56adfe4087a8..5d26dffe5c0d 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -4250,17 +4250,12 @@ static int rtw_test(
>  	DBG_88E("+%s\n", __func__);
>  	len = wrqu->data.length;
>  
> -	pbuf = kzalloc(len, GFP_KERNEL);
> -	if (!pbuf) {
> -		DBG_88E("%s: no memory!\n", __func__);
> -		return -ENOMEM;
> -	}
> -
> -	if (copy_from_user(pbuf, wrqu->data.pointer, len)) {
> -		kfree(pbuf);
> +	pbuf = strndup_user(wrqu->data.pointer, len);
> +	if (IS_ERR(pbuf)) {
>  		DBG_88E("%s: copy from user fail!\n", __func__);

This printk is out of date now that we're not using the copy_from_user()
function.  It doesn't add useful information.  Just delete it.

regards,
dan carpenter