Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fc3e8a40-664f-07ae-7474-c0412a1ab1b5@intel.com>
Date:   Tue, 10 May 2022 00:22:55 +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>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        <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>,
        "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>
 <a339df59-9e00-c7cb-e33d-2ac626443639@intel.com>
 <3b7fe4fe-fdec-cef2-4e0e-309d9dc4a8af@redhat.com>
 <b5ab1c49-04e7-36c3-677d-2989b79e50ca@suse.de>
 <2bf27b09-0896-1849-254f-d5b19abdc892@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: <2bf27b09-0896-1849-254f-d5b19abdc892@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?R3JBdXhvTDQrWTNtSStUZHphajRBM1VPOGtGb3NQczVxSXFmR1A5L21pdGFY?=
 =?utf-8?B?WjZrWTdDbEN4R0wxdS9ySnlFbDVTKzFBcytBYjhQeFVoVWZzd2l3dU4xUlJu?=
 =?utf-8?B?bmUxYVpGZy8yTUJDQ1Z6dzVucFdQU3hwSWxUbzR4OTRPUFBxejRleWYxTmNZ?=
 =?utf-8?B?YlU1SC9ySzJyN01EWXJqbGR0R1VCaEx1WEtnYkErZC9WRnZFNkFmTE9nS3hY?=
 =?utf-8?B?R3lQVjBPdmg2TE81MCsvRHpqeXVYOTNlT3ppeHlacklocFNRU1Uvem0rQXpI?=
 =?utf-8?B?VWdReEFQcThzRjlWUlFJOUdzN3VpVkE3c1dlb1NWdWhyL1pIQUcyeG1nQnJr?=
 =?utf-8?B?L3VzVks1Wk9VSEExMzViOG5tOVBBRzIrQ1FIL3hLSlMwTEJnZkdVZThId3R5?=
 =?utf-8?B?ZkpBYkYwbTFnek5EZDVqWWZTNmIxeUZIS0JXRTJmeDBvYm8zUURHNlFTRG5X?=
 =?utf-8?B?NTBiZmJhTDdiVFgxSndWTW5GS2ZzeVZwQUpwOXloMW5aR1Q1SURaR0w3T0py?=
 =?utf-8?B?UGRsbUpXTjQ3KzRtU1NqbVNnR2liRm1UejR4Ny9sWHd1OWYwM2NRQ2VOOFNu?=
 =?utf-8?B?eFFiV0xlMmk0VVg1SHRxM2YveUpzZXBYc09PR295UXRLQ09xUGowbjI1NzhY?=
 =?utf-8?B?NFNDcFlybEZSa2pKYUlqZGp3NXZtR0UvWGhNWHBLcTRVWk5ZblZiVmtvcGdM?=
 =?utf-8?B?RzJycm5XMzc0VWswOG9IeWhHZ1lCaEJubkxydkcyaE1MSUZJc2doSzZQZXkr?=
 =?utf-8?B?Umo2NnVFSEk2VXRkL2ZrLzZDdU05UkNhZ2kzelQwSXA0Q0RnSGhFR2xtWTdo?=
 =?utf-8?B?Q093T3NXUmMyOS9BeW5oanVYcnl6QW5taXJNWkEwVFkvd3RmMFZUZU05MzlI?=
 =?utf-8?B?UXBIWk9BWXNaMkRCWGtUZWxvSHpMa0oxaUIvelVGZjRLUWtNYXlzMXU0Ly9i?=
 =?utf-8?B?ZG43WjRwTmlvWVphZ3ZBQTIrRkVkdkIraWFxYml0SEhxMG0wNUlNZEZCcGdm?=
 =?utf-8?B?QXljdjlESDlIL0dsZ0lDbm50bm1oci93dkp0SENCQ2NCajhZYm43MHduZytr?=
 =?utf-8?B?dWxwam1CazYza3pGTVpwcVNZU3QwbGJmRzhpRDIxTjRtT1F3S1A2NkZmQmJy?=
 =?utf-8?B?Z2xIWEJJSktkVkVabmpzVURVSC9Db3ZvQUFmKzdBR0liUUpnVUZjL29kSk15?=
 =?utf-8?B?NURJbzFzVTlZVWJVODlkNkd6UHJkWStEaEh2ZHVnaTFyR1MvaTY4b1diZE1K?=
 =?utf-8?B?aVQ0SkxyU2ZSc25lZ0VEVDZ3U2M0T2ZZbUxObTdIdEVpTFFzTGNqMWVXcnds?=
 =?utf-8?B?S2J0WFdDbXQ4L2NxNFluVXhMZHcvNEUzdnRlNDZPSG56c3NLVXVWaGMvVDVo?=
 =?utf-8?B?VWhocUo3KzJwQXlhVmVxeGdKaFpXV0hzaHNxOWExUG5UamE3RnBoWHBqNWR6?=
 =?utf-8?B?NFZPOVJkczJiQkxrenhkbFVmVzVSVkhLTW9TdDVSTytBNlJtRGdWZmI3emth?=
 =?utf-8?B?dmtvNHBpTm1yRzVOTlZpeEp3VUh5b0oyQ2padHlDdERwaDFqSWNxY0lCaHhG?=
 =?utf-8?B?K3VoclRRYXliUEsvMjQvY1RtZm5WWnFBQXVUMTJUN2ZPdEJKNGExOWMrUFVI?=
 =?utf-8?B?Mk9ZVmNqTmdvRTE1TndDbWIzaGJuMkI1RCtpcVFsM2lSTHNXTHU0c3d3Vjls?=
 =?utf-8?B?TEFWYndPWDg1eUN2bmdlSDV0bEJqVHN5RXh2UEJKaURxa0tsVGxPbit2UVUx?=
 =?utf-8?B?SWk2Lzl6bm5SeGk4cGdsb0VYV0dTTXI3UStjeDRlay9tVlRyWHVGRHIzajVl?=
 =?utf-8?B?N1lVTEUyR0dDVUd5VTMwMS9rTkNlRHV2dENQZDdrVUZaNEN5NFFKZzViUjdL?=
 =?utf-8?B?SmNRV3BhT1J0OTRUUVdxOURXb1dmVEpIbkNkQWpXc2tEQXJrVlZvSFR3MVpN?=
 =?utf-8?B?Wm1IMXYzN2ZSMnkyT1NlYVdNU3hkNzNDaXhrdFRxaklLaEtITXZkY1V5c0RZ?=
 =?utf-8?B?b1h6UVpGcytHMG5JclRUVERkSVd2RnM5QncxRDBUV3Rjb3RYR2xhN2djOWNl?=
 =?utf-8?B?NGk0S3JIZmNuY1RJLzIzK3haK2tqazcvQTU1Kzg1WUY0czlRMFRQeEZrYWow?=
 =?utf-8?B?YmlEUzlYUVhDWmlmV2lteTZDRHk5ZnJia3NTencvbFVhRmZ0dDZVckc3c2ZG?=
 =?utf-8?B?THN3d2RJaTZYdm5lanF1UUtyeStrdEhycUY2NXU1c0N2b3R6dDdyRWdzaHdP?=
 =?utf-8?B?enVsSWQ3QkVEZlljaWNtQ3hJM2FQOVlTUlRNdmMrRGQ0WnJmMmp6c1V5aEdq?=
 =?utf-8?B?blFSUVRHRi9tM0JpQUtSM0pmZzJlYkR2SXpkL2tXbkMzc2tWMjVoWjdLSmMv?=
 =?utf-8?Q?M4NDy94iBtyB/1oA=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 70bcc92a-89c5-4256-c373-08da320a7afe
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3180.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2022 22:23:02.3112
 (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: 4Rn1ajGKRQgvVcG/8GZ3mkURqyG93UHWx+VdW/sethRrnTujHs7QwZutI3zBCpMEeCnh48KBcS8seFXb96ejXQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB2583
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham 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 22:03, Javier Martinez Canillas wrote:
> On 5/9/22 20:12, Thomas Zimmermann wrote:
>
> [snip]
>
>>> I actually thought about the same but then remembered what Daniel said in [0]
>>> (AFAIU at least) that these should be done in .remove() so the current code
>>> looks like matches that and only framebuffer_release() should be moved.
>>>
>>> For vesafb a previous patch proposed to also move a release_region() call to
>>> .fb_destroy() and Daniel also said that it was iffy and shouldn't be done [1].
>>>
>>> But I'm also not fb expert so happy to move fb_dealloc_cmap() as well if that
>>> is the correct thing to do.
>> The cmap data structure is software state that can be accessed via icotl
>> as long as the devfile is open. Drivers update the hardware from it. See
>> [1].  Moving that cleanup into fb_destroy seems appropriate to me.
>>
> I see, that makes sense. Then something like the following instead?
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d265a73313c9..ce0d89c49e42 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -627,12 +627,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>          cancel_work_sync(&fb_helper->resume_work);
>          cancel_work_sync(&fb_helper->damage_work);
>   
> -       info = fb_helper->fbdev;
> -       if (info) {
> -               if (info->cmap.len)
> -                       fb_dealloc_cmap(&info->cmap);
> -               framebuffer_release(info);
> -       }
>          fb_helper->fbdev = NULL;
>   
>          mutex_lock(&kernel_fb_helper_lock);
> @@ -2111,7 +2105,11 @@ static void drm_fbdev_release(struct drm_fb_helper *fb_helper)
>    */
>   static void drm_fbdev_fb_destroy(struct fb_info *info)
>   {
> +       if (info->cmap.len)
> +               fb_dealloc_cmap(&info->cmap);
> +
>          drm_fbdev_release(info->par);
> +       framebuffer_release(info);

I would put drm_fbdev_release at the beginning - it cancels workers 
which could expect cmap to be still valid.

Regards
Andrzej


>   }
>   
>   static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>