Date:   Thu, 11 Nov 2021 13:33:03 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     laurent.pinchart@ideasonboard.com, mchehab@kernel.org,
        ribalda@chromium.org, hverkuil-cisco@xs4all.nl,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] media: uvcvideo: Fix a memory leak in an error handling
 path of 'uvc_ioctl_ctrl_map()'
Message-ID: <20211111103303.GQ2001@kadam>
References: <95f3fd02313ff41d6808b8e1f20e0c582f46edc8.1636617903.git.christophe.jaillet@wanadoo.fr>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <95f3fd02313ff41d6808b8e1f20e0c582f46edc8.1636617903.git.christophe.jaillet@wanadoo.fr>
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?zY2SPsY5kXblZ4HBpRwmQn9lRhZlEXGZUIFOcpWakyVz8K0csJxa+yClPEz1?=
 =?us-ascii?Q?zivs1kQn+UTudwO5FzwEwmQOHujLx6sjoxb5v3xV68jWpG6wTYouVmrKsNWV?=
 =?us-ascii?Q?57dFexahlKJ9pJEuarSdbwKsdoTgf7x86B9GH9VLQk5XAg5gMGkcOjvZKHJO?=
 =?us-ascii?Q?RJnzbwf3LDbEZhoaRQwmluixUKTIzvzPZ6LkkR0Cj1Cw+ThcJilsd3zGhNrv?=
 =?us-ascii?Q?Ia/PrCJMk/2TvuPQEe1hYuM7ac02RXKwff0Yuve0UlJjKtVa+nb0akWti3lC?=
 =?us-ascii?Q?tgOjOPkyGFVpZqkgpqOz6KcHOt00mh1lpXvfXNI3+uMTJCvKPAc0tE6bLo9v?=
 =?us-ascii?Q?7o43djIxOf0IP/VMWiZXYCqb8ouDYJZ+WQM5Uvc/9KJrnpfsMNPg65Bb6idH?=
 =?us-ascii?Q?HHB8cdSyslHGnzMxvAi0tEkj44weGItbmGsR0FcZpWWS3/1lx5zzEzBawJM1?=
 =?us-ascii?Q?6N+H4n7+0mmPaUjkxeT6y435YmXBW7shDKPMqTsrNnZpfv6tgfDfDIDW6ZmQ?=
 =?us-ascii?Q?bNwj6X0bfQ2lzsmd6ez7ptLdPdnE5wKAkv+lCTvSm4Bs70R15Ff8Ybda1JqB?=
 =?us-ascii?Q?6ZFvIp7a2wBzX4P/uHIL7+zbYrdRdNiDc0nq1LbaWadStWUAp3NbK0nc31N6?=
 =?us-ascii?Q?aJXW6l4pulC0Q+plzbcKvaShWiePM0NA8HdydpopACbGDZyt6VofetSRHDXA?=
 =?us-ascii?Q?gve6YoBLIqmcnTUzW1iCEd62KjjW+iROoq4IyIlRmGwQvxaB7OTOvYp0oil4?=
 =?us-ascii?Q?JF67A3J5kmnAFlZLqteFemm55YBnvaanzURNOxYZtKiKj84Et1GXnwKVQP3B?=
 =?us-ascii?Q?JVLrHHJk9rcTeZpNTfCZPlHmR3D65UItv1QIiR5IeKAGBaFL9dtJkC4eFIKa?=
 =?us-ascii?Q?HhrFSWdzoY1db0DqcsqJ02p0zQYSu/1FZS7PrrsYEsdMdzBFpWgqsIeXlLAJ?=
 =?us-ascii?Q?SC89TLdfIzwt2UYs8zgJFbX7BD9/8/mhGv6ENKnO2n3Ql7NBxFkWTWTxebgF?=
 =?us-ascii?Q?RbhpoNcjNO0vl9RVWUiITX0oQpNU0g9DOHLHEBpZtIpC1YmdNK+P7a4kkSa8?=
 =?us-ascii?Q?HoVy+2c+k9MZzSsG/bErSQprzHVL4F/x6UoQq4mHelZIV+hpsBOTInBrUiVs?=
 =?us-ascii?Q?fMi/6DJD2MEGMHfsP9nGtVCLHZAr/njmRz+J5Mk6fHHl9podsBR6fRnR8Ia5?=
 =?us-ascii?Q?s/B8B93w9+GJDaNjlylqbslnvbsb7a9mGp2T63hW3f4D4aIQ2+B6Z83QmNIa?=
 =?us-ascii?Q?JL8PDbsTokYKnFe9HXPwcgT1Pmt/RB3q16YRJOclEuGp85dosr8GLfvly8sq?=
 =?us-ascii?Q?1zRXMdd+5ov5bUdWK+VMpv8/KF3GSfDL3K6SxIfbV/e3Xtha+UBZPOjThcLD?=
 =?us-ascii?Q?34lqmM5Z+hONQHDjYstMmYOfDdjAELMCQxHwO0uSpDWLIWSCtnx/dJzPWfqn?=
 =?us-ascii?Q?3dWPyOSsBacWBVWFFGQWLyQIiFrrBB4x/RQzwF7aakkvIclutSsdAkEeI4Ez?=
 =?us-ascii?Q?ZRswh9q/nOPm1U/QVTqa7TQc5sFYMt0Sryt/vHw1YGJDNMbOw4Gk7xp4RNTm?=
 =?us-ascii?Q?JVnet7Bo3TS/tv51GdmiBdpmZFGvlOrZCORL7BeuCdxJLh9MTNlAa9wuLRBV?=
 =?us-ascii?Q?zp2cLPRD/H84FB0U2Qz630WYOAVhjFiKmFG+5U5Ti5IpVpeKrtNUWbvq1O3x?=
 =?us-ascii?Q?0FUBdyIZ4mnCVbqTTTtl/lessYY=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4dd02e22-7f2c-4300-057d-08d9a4feb430
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Nov 2021 10:33:30.6897
 (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: N78KbV6KytQAkltVsq8Byozk8LjcxPzPz+fwAum2htUt8KFw7EdJxvFrgL68FUsl2tIT+O35qFQb5QxB9OGkqKzlIyLTVTwHd4fV9t0BjKA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB5861
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10164 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=930 mlxscore=0
 suspectscore=0 bulkscore=0 spamscore=0 phishscore=0 malwarescore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2111110064
X-Proofpoint-ORIG-GUID: dMwQwZqfCDv7S4M90p9PQ-qdLIZBN2DI
X-Proofpoint-GUID: dMwQwZqfCDv7S4M90p9PQ-qdLIZBN2DI
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Nov 11, 2021 at 09:06:11AM +0100, Christophe JAILLET wrote:
> If 'map->name' can't be allocated, 'map' must be released before returning.
> 
> Fixes: 70fa906d6fce ("media: uvcvideo: Use control names from framework")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4e4aff8ddf7..5aa76a9a6080 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -44,8 +44,10 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
>  	if (v4l2_ctrl_get_name(map->id) == NULL) {
>  		map->name = kmemdup(xmap->name, sizeof(xmap->name),
>  				    GFP_KERNEL);
> -		if (!map->name)
> +		if (!map->name) {
> +			kfree(map);
>  			return -ENOMEM;
> +		}

Your patch is fine but there is a second issue.  The error handling
should free "map->name" as well.  The problem is that this function
frees everything on the success path at all, but freeing map->name on
the success path will lead to a crash so you have to do something
weird like:

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index f4e4aff8ddf7..953a5cbf7945 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -90,6 +90,9 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 	ret = uvc_ctrl_add_mapping(chain, map);
 
 	kfree(map->menu_info);
+free_name:
+	if (ret)
+		kfree(map->name);
 free_map:
 	kfree(map);