Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <753d0350-42dc-389b-b10b-4533ddcf32ac@intel.com>
Date:   Mon, 9 May 2022 16:56:23 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.8.1
Subject: Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in
 fb_release()
Content-Language: en-US
To:     Javier Martinez Canillas <javierm@redhat.com>,
        <linux-kernel@vger.kernel.org>
CC:     <linux-fbdev@vger.kernel.org>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Helge Deller <deller@gmx.de>,
        <dri-devel@lists.freedesktop.org>,
        "Thomas Zimmermann" <tzimmermann@suse.de>,
        Daniel Vetter <daniel.vetter@intel.com>
References: <20220505215947.364694-1-javierm@redhat.com>
 <20220505220413.365977-1-javierm@redhat.com>
From:   Andrzej Hajda <andrzej.hajda@intel.com>
Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298
 Gdansk - KRS 101882 - NIP 957-07-52-316
In-Reply-To: <20220505220413.365977-1-javierm@redhat.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WXNnRXU2eTZaR3ZxMzhZR25xQjlIWlk0VC9leVd3UVlsbmtCbUFtTWNEMStW?=
 =?utf-8?B?b3ZBOVVKMXhsdTZmQnJRa0RoMjdaNlgxbE83ek5SR09mOVNjL1NIL2pMdlJK?=
 =?utf-8?B?YXJmOGJLeEJZdndLb0c0dkVxTDNlUVZicUo4ckpweU5XalNNbDFma1UvWWl2?=
 =?utf-8?B?ZlY1a0hJNVJ1bzNrRWVIdUhjTXRobTlwTzZRRVlVYW5scEhsNDFuTmVlQXVS?=
 =?utf-8?B?RG9rQlVMUG44UVNwVGtMVjE5TTNidDJibHI4RlNYVytIcm5LK2NkZXo2ajFB?=
 =?utf-8?B?RDZvUVJ5dGFrZUx6Nk5VUFA4bmY2VGQweEN0Qzg5K1RjZzU0U0tpS2R2cFJm?=
 =?utf-8?B?THhkRExRZEllS1MzUmpoR2ZMaWRxR3l1UVlWalYxU1J2MS9jYThaczY2NnQx?=
 =?utf-8?B?SmdIdEZ2U2VzRGppaldqYUJjSm5PQ2tVekMzcEFCOWZqQ2pJeS9KL2NZSGR3?=
 =?utf-8?B?bjFMRTliYkpYS3YrMkVDaHByNERnN0JNeGVrNUZqZWhVSlUvNEdoTXB0K2tB?=
 =?utf-8?B?NFpveThPc3I1WUdNZXhMWnpFY1pJMkxsVUE3K2Q5MzBSUmZ2bjBrdEZZVTh4?=
 =?utf-8?B?a2QyRGV0NEdTTzRsTG9jcHg3UE9SUFRHQWlvYkFDdXA2amR5UWRrcmlUSHFK?=
 =?utf-8?B?TWNwVVl6SnV2Nkw1SHlXRWZCbFo1U3NHbkNjWWZqMGVTRXJ6cHhnUFcva0M5?=
 =?utf-8?B?STZSMmFYclgyakc0a3dHRnZRQ2E0aUp6dnZZTjAvRW8ya3hlLzhJR25POEZz?=
 =?utf-8?B?dUM3aUlPUXJaOWd3VVB5dzUzVjBNZmxoZHdWc3diWndSNTNOeXMwYzZNTFZ5?=
 =?utf-8?B?RTVkV3A1SW9RaUhpVWYrUnZNVWx4YUJFZlBUbUFOTENPVGRvMDJ2cklCVTVa?=
 =?utf-8?B?THBqWlZwakxDY2xJVVczWmNzR1l6RWQ3UDNWelpFZk93Rm1YaFluc0hQNlQv?=
 =?utf-8?B?REFxeVRUTHNMekU2aFBSSUxFNmFST1NMekFLODE2SVVGQTJPeEFXL1NSZ1ZR?=
 =?utf-8?B?d2hZVHA1aldaRGl4S3pDTVNvOEY0dHRTWjZMcUJqeUZxTStneHIycXpJNWd1?=
 =?utf-8?B?QkVtU1JidlZaRDB6eU9EdEhrNGVoazNSVXAzNWJzcDgrbFE1YWR2QzB6bzh2?=
 =?utf-8?B?anFSTXh5K2dpOGxjemwvYzRDV3Y3a0FQVkgvblJLYngzYzVNdnNvdVBaT2xH?=
 =?utf-8?B?cTVua0xHdGNaTnZFdWdDeUh6N3NiTjRldEdBVEhvcmd0SWFtcXhDNUxrMVls?=
 =?utf-8?B?Z0xzaVkzMGZidFdDcTh1V2VubktXblhjRXZCRSt5UlAvQ3BXb3YxY1lvejdI?=
 =?utf-8?B?Rmc0ZHJmUEJ4ZVFZYUx1eHUvWFhnTXU3V2ROVG9SQTdWaVBNd3M3SHlHZXg3?=
 =?utf-8?B?TVdKSjJnZk9mcGtWMEl4ZldjeHV4aFlwK3QrWWs2VjF3b0hYRkRBOHR1YjdT?=
 =?utf-8?B?ZDk4UEgva3dxY1hFOE03cTQ0cmxISUFwWnFMRlBDSGZnRlVnSyt3SU5hTk1T?=
 =?utf-8?B?U2hJWmx3b1VtMUsyV3Q5dHFLaUNvaHkzdGFDVHE3OXljVWU1Vnd5L0l3elN5?=
 =?utf-8?B?U28vU21sdThUd1FFc0dOMXV5d1BmT0loc2VSb0JqT21zRmlzYWxnSlJ4N1Q2?=
 =?utf-8?B?OHlvaUgwZi9mRnBPTHhBb2RKSmwzallQV3JveUZRN0JvN3F5cExXTWIyWHdR?=
 =?utf-8?B?ZEtFNmp2TG9TN3RqVGtndDhwS0hGYVBRelU0dTFhaEdwNkVKY2xnVCs1WGhi?=
 =?utf-8?B?Y1RFbElYbys0WU9yTVQ1VTNpVVdyWkl5L2FLUG5RZ05wY1VmNkpBbEhYTURL?=
 =?utf-8?B?cUd6Zk04TG9XWnZYcXFIOUNKUnRQZWdxZHF6TWVsa3JSVzMwbFNhaU9OMGVF?=
 =?utf-8?B?bnVCWE5OazAzSFlMOUpnQ2dGd0F4cmlpbm43WjlMa21rbkpGdTVlQlo4TElt?=
 =?utf-8?B?Z1hUd3E0TWMxMFNYSmp3eU12STJ6K3Fxd1hpZWsrSjJ2emZ5bElwYUNpRzZ3?=
 =?utf-8?B?WFZGcS9HQytkSGRXVUtkQWRFeWJIeGhicWdha1krY29UZG8yalE0S3BoQWNy?=
 =?utf-8?B?WjdQK3RLS1VBZDc0bm9NcGpNNGxPS2M1UGtaTmhleEJ3Y09kbG9Jb1gvNHpo?=
 =?utf-8?B?SkxwSkx5K0JSMUNOSU1xSFcxSU96UVFQSTBmdWNkcXVTbm9nTFl4OXhidkpj?=
 =?utf-8?B?Sldmc3kwMUIwRFlkbU12MUpKMnFFN1lsWjZUMnFJUVR0RmlSVXI0SUgrTVBE?=
 =?utf-8?B?eWJ5WEdlS3NNU3VCVlk5S01lcmhaMGdYZXpLbWRtYXI3VHVKellvMkpuVkNO?=
 =?utf-8?B?N016SHNha09pdzJMMDVibS8zNGxGRHF2a3VXU25jSVc0VEFwY0YwSDdKRzBm?=
 =?utf-8?Q?/HOwpXXtOt33I2Bk=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d78f92dc-5226-4f30-fcd4-08da31cc1865
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3180.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2022 14:56:28.1741
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jPcTPPRb8m1EEsdQ7IMpTxg5u+M8NCIymFOAlg/jEQ2ByezU+XpC/QqNY/9GGpfxhtbZdkfRs7vSuhcQDmpSXA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5356
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 06.05.2022 00:04, Javier Martinez Canillas wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Most fbdev drivers have issues with the fb_info lifetime, because call to
> framebuffer_release() from their driver's .remove callback, rather than
> doing from fbops.fb_destroy callback.
> 
> Doing that will destroy the fb_info too early, while references to it may
> still exist, leading to a use-after-free error.
> 
> To prevent this, check the fb_info reference counter when attempting to
> kfree the data structure in framebuffer_release(). That will leak it but
> at least will prevent the mentioned error.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> 
> (no changes since v1)
> 
>   drivers/video/fbdev/core/fbsysfs.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index 8c1ee9ecec3d..c2a60b187467 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
> @@ -80,6 +80,10 @@ void framebuffer_release(struct fb_info *info)
>   {
>   	if (!info)
>   		return;
> +
> +	if (WARN_ON(refcount_read(&info->count)))
> +		return;
> +

Regarding drm:
What about drm_fb_helper_fini? It calls also framebuffer_release and is 
called often from _remove paths (checked intel/radeon/nouveau). I guess 
it should be fixed as well. Do you plan to fix it?


Regarding fb drivers, just for stats:
git grep -p framebuffer_release | grep _remove | wc -l
Suggests there is at least 70 incorrect users of this :)

Regards
Andrzej