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: <a339df59-9e00-c7cb-e33d-2ac626443639@intel.com>
Date:   Mon, 9 May 2022 17:51:36 +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>
 <753d0350-42dc-389b-b10b-4533ddcf32ac@intel.com>
 <1f788b8f-0bea-1818-349e-b1bc907bf251@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: <1f788b8f-0bea-1818-349e-b1bc907bf251@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?UlNSR2NaMnZCeWJ0QXJMOGY4TllmbkRPVVY3a1hBS3VmNVcraE42MGFTVXh3?=
 =?utf-8?B?UHZSMDFaWExYeW1sZVRCSEpMNmtON1dnbUZMbnlqMTVVWDlFWm9NYTdkdmtD?=
 =?utf-8?B?ajNQUmtYcnFyb1oxT0xRa2poam1VSFpxSHNVSzJwSWRTbGJESSthbEJJSmI3?=
 =?utf-8?B?QmlHbkxNL20yWVJQV2xyWTVZTU0xR2YxMkN6dUVTajF5TWErWitJL1Q3TjZQ?=
 =?utf-8?B?RHJVOXduVjZkZ1FjY3haWGYyMXlCOWlMWVN1NWRzMHQ2UlZWN1Q5K1NrQTNw?=
 =?utf-8?B?YTJzSXNZU2RzelViamxWUkNVdFVIQ3QwNHZxY0s4dDNKM3NTMnF4TXB5RHhB?=
 =?utf-8?B?eEVIY3hpbjN1VTB1c0N6YXRsdTIzUnZuNGJ6NVd0ak5ad2IwNGdXWm1EeWVC?=
 =?utf-8?B?eHIzd2ZySnNZM2FkTGdKUE84ZFhIc2FVMXRhK25WQTZvL09STThmc2dIZXRC?=
 =?utf-8?B?WHpDQTZkRlUxbjV3eGhvQXNtc0Z4V0VSN21LOXFoLzBuYzdDd3NadTE3TElh?=
 =?utf-8?B?djltNWhYRUMxenExOFB5OEt5V29LdFVoQzlIaUpPbDhGOVBPVGVYa0wzRGo2?=
 =?utf-8?B?OCtyTEpBU0dCcmpac0Zka2s4U2d3Vyt4ZTZMUTY1UVZObnFVL0VxdmlaaWk2?=
 =?utf-8?B?UFhmeGwzS0JVY2oxaTVxbDVnRXJ0UVlMQjlGSGFRelY3akk0NVA5Slo1Qjly?=
 =?utf-8?B?Qkgyck5QSmh6YUNua0RZdVYvNUcwbjBtVEJZS1BMVzdXQk91ZDhBT0o3SXYv?=
 =?utf-8?B?VGRCWVZjWVFRZllnV3ZUNXJQcGpyR2k0VDVtK2J1a2h2OVY0OWw4Z1E5MkFs?=
 =?utf-8?B?MHRFV2pvL09GMFVHZW5HOTlDaktmZ1NhRjRxWHBlUG9qcFkzQ014c0ZIUFpC?=
 =?utf-8?B?SHNpMEdyR3lTQnIrK2ZabGE5Y3QyZHFTQ3NjYmhISVJOVDZkTW1LVU1RTFlu?=
 =?utf-8?B?ckp2cnV4MGpiVVJTZjVhbWxGTytLdnJxN2dMdnRMdGQvWEFVRzFaVmpGaDRs?=
 =?utf-8?B?dk03QWV1UjYrbkJBZW9jSlpaTTFFMGE2cVFtb0RIL1V0WXRXYUN4WUZ4NDRE?=
 =?utf-8?B?Wmx1cWpNSEFVQU1GNlY5T0FPaVRsWTNRNU5NZitkOUtPZ1UxMkVDSjdQUjcz?=
 =?utf-8?B?YmkwU3FlalptV3FWcGhNbzUyN1NzdkdqeG15dm1GeGVVM25ZcXBQb2I2NTBn?=
 =?utf-8?B?SFY2NXI2RGd0SWpmN0Q3WWE4MlhWcEVCUW9idHNvR2NNcHdxL093WElIQTNF?=
 =?utf-8?B?STRiamZpS3grSm4rRFNKN1ZDL3NWdnZ2aGtPVG5SKzVjVEhrUnBSd2MvbGEr?=
 =?utf-8?B?R3dxaUowcjdmb0VUWEhTVHpXN1ZqNWJUalNoRTl2Y1lFdm5FbUxlYU9OWC95?=
 =?utf-8?B?VWVqT01wN1VKcXY4MkdEY1BMdVVMbWtzVzJoYUs2dnYxOWFVNEU1MHVaZS9a?=
 =?utf-8?B?NWZPeFJZQWxvZVVnRHR6SlEzVjRndkpiMHpDdXVaQUd2OXZLcHM0VGZWMjJH?=
 =?utf-8?B?OHVJbjV4WDlZWDBWTTRtbm1NQkpkcGVOR2wwRCsrWmJpeEx6ajd3a3d4TjZX?=
 =?utf-8?B?UWZoQ2YvSlJqTlVOd0JZbFhOYkJQQXR4SHQyZ1ZXUzZNaUpZSGpHZXJZKzJO?=
 =?utf-8?B?NFh1UG5tU1NsOUhIRzBWYks2QXdnVlQ1Q21ReHFNR01QcHFOWUxTeUpSY0F3?=
 =?utf-8?B?Mnlic29IUzhFckQyYXNJWjE3UHUyWXoyTXFMRlg0eHFaMWhSTG1YNFBoU280?=
 =?utf-8?B?M0tJVjFkaUVwY3lwZW9KK0w4NytIS0cvOFBMemJFKzlOUFR4aVZBbVNQZm1K?=
 =?utf-8?B?Qzc2b1daLzlFN2JXT09sZUh1RU9PMEIzK29URy9rMFY1QjdzNHduM3hHNjFF?=
 =?utf-8?B?Q0paVmlyTHZiUmZ6elVLUEY0T01hYmlTR0NCakNwUzBMNE9tSEJiWUdIanR4?=
 =?utf-8?B?TXoyaEpNOTJnbGpzMXlBLzRDN0x5anNNeFhVMGxhUzZHTXM4UUpHenRtTTJ6?=
 =?utf-8?B?eTRtZG5WYkZUbW9FRXBKVVVEMGJzd1dCSTJ3TFk0cVhwL2h3U3NDMmxBQ3lG?=
 =?utf-8?B?NzJJWFhqTkdzSCtCcVBVdmk0Q1luVi9oRGxPWTJsdXpJazJGMDZMWkVqUkZs?=
 =?utf-8?B?SW51cHZGcEkySU9qQXBuOEdVbVlYWS9seUJSOEdDaUtra0dIUkJ5eUk1Rmxa?=
 =?utf-8?B?RUI4amREQUFnTVhiRWxCSHo3REE3SSs1V2g1UVJXTDVmNDh5THFvYzZ5QnQz?=
 =?utf-8?B?a2VKTklBQlhDWE1BZnNkQVhiTkcyTjRnWnBBVlBneWhjQ3A3cDJ6QmdsNHBG?=
 =?utf-8?B?K3pYVzFWNzlnTVpiV3ZiZkRtKzFuRjJBWnVFa3FETjlyMGRQL2t3TkR6b3Jn?=
 =?utf-8?Q?2yeqtDgL5zcE8Xpc=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a1487ec2-3b6c-4c62-7fa9-08da31d3cfd5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3180.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2022 15:51:42.2957
 (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: +s0/R086I0/Uxr93Ntl3Tm+l53WxroDULsSaH3RSLTxElTWmepnbONJdUk9gFBcbFGl86qRSvrrT4ySZ0kbpkA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB2942
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 09.05.2022 17:30, Javier Martinez Canillas wrote:
> Hello Andrzej,
>
> On 5/9/22 16:56, Andrzej Hajda wrote:
>> 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?
>>
> I think you are correct. Maybe we need something like the following?
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d265a73313c9..b09598f7af28 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -631,7 +631,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>          if (info) {
>                  if (info->cmap.len)
>                          fb_dealloc_cmap(&info->cmap);
> -               framebuffer_release(info);

What about cmap? I am not an fb expert, but IMO cmap can be accessed 
from userspace as well.

Regards
Andrzej

>          }
>          fb_helper->fbdev = NULL;
>   
> @@ -2112,6 +2111,7 @@ static void drm_fbdev_release(struct drm_fb_helper *fb_helper)
>   static void drm_fbdev_fb_destroy(struct fb_info *info)
>   {
>          drm_fbdev_release(info->par);
> +       framebuffer_release(info);
>   }
>   
>   static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>
>> 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 :)
>>
> Yes, Daniel already mentioned that most of them get this wrong but I was
> mostly interested in {simple,efi,vesa}fb since are used with "nomodeset".
>
> But given that I only touched those tree and still managed to introduce
> a regression, I won't attempt to fix the others.
>