[PATCH] drm, fbcon, vga_switcheroo: Avoid race condition in fbcon setup

[PATCH] drm, fbcon, vga_switcheroo: Avoid race condition in fbcon setup

[Thomas Zimmermann]

Protect vga_switcheroo_client_fb_set() with console lock. Avoids OOB
access in fbcon_remap_all(). Without holding the console lock the call
races with switching outputs.

VGA switcheroo calls fbcon_remap_all() when switching clients. The fbcon
function uses struct fb_info.node, which is set by register_framebuffer().
As the fb-helper code currently sets up VGA switcheroo before registering
the framebuffer, the value of node is -1 and therefore not a legal value.
For example, fbcon uses the value within set_con2fb_map() [1] as an index
into an array.

Moving vga_switcheroo_client_fb_set() after register_framebuffer() can
result in VGA switching that does not switch fbcon correctly.

Therefore move vga_switcheroo_client_fb_set() under fbcon_fb_registered(),
which already holds the console lock. Fbdev calls fbcon_fb_registered()
from within register_framebuffer(). Serializes the helper with VGA
switcheroo's call to fbcon_remap_all().

Although vga_switcheroo_client_fb_set() takes an instance of struct fb_info
as parameter, it really only needs the contained fbcon state. Moving the
call to fbcon initialization is therefore cleaner than before. Only amdgpu,
i915, nouveau and radeon support vga_switcheroo. For all other drivers,
this change does nothing.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://elixir.bootlin.com/linux/v6.17/source/drivers/video/fbdev/core/fbcon.c#L2942 # [1]
---
 drivers/gpu/drm/drm_fb_helper.c  | 14 --------------
 drivers/video/fbdev/core/fbcon.c |  9 +++++++++
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 53e9dc0543de..c0343ec16a57 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -31,9 +31,7 @@
 
 #include <linux/console.h>
 #include <linux/export.h>
-#include <linux/pci.h>
 #include <linux/sysrq.h>
-#include <linux/vga_switcheroo.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_drv.h>
@@ -570,11 +568,6 @@ EXPORT_SYMBOL(drm_fb_helper_release_info);
  */
 void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper)
 {
-	struct fb_info *info = fb_helper->info;
-	struct device *dev = info->device;
-
-	if (dev_is_pci(dev))
-		vga_switcheroo_client_fb_set(to_pci_dev(dev), NULL);
 	unregister_framebuffer(fb_helper->info);
 }
 EXPORT_SYMBOL(drm_fb_helper_unregister_info);
@@ -1614,7 +1607,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
 	struct drm_client_dev *client = &fb_helper->client;
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_fb_helper_surface_size sizes;
-	struct fb_info *info;
 	int ret;
 
 	if (drm_WARN_ON(dev, !dev->driver->fbdev_probe))
@@ -1635,12 +1627,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
 
 	strcpy(fb_helper->fb->comm, "[fbcon]");
 
-	info = fb_helper->info;
-
-	/* Set the fb info for vgaswitcheroo clients. Does nothing otherwise. */
-	if (dev_is_pci(info->device))
-		vga_switcheroo_client_fb_set(to_pci_dev(info->device), info);
-
 	return 0;
 }
 
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 7f35ad66b462..863944530c8e 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -66,6 +66,7 @@
 #include <linux/string.h>
 #include <linux/kd.h>
 #include <linux/panic.h>
+#include <linux/pci.h>
 #include <linux/printk.h>
 #include <linux/slab.h>
 #include <linux/fb.h>
@@ -78,6 +79,7 @@
 #include <linux/interrupt.h>
 #include <linux/crc32.h> /* For counting font checksums */
 #include <linux/uaccess.h>
+#include <linux/vga_switcheroo.h>
 #include <asm/irq.h>
 
 #include "fbcon.h"
@@ -2894,6 +2896,9 @@ void fbcon_fb_unregistered(struct fb_info *info)
 
 	console_lock();
 
+	if (info->device && dev_is_pci(info->device))
+		vga_switcheroo_client_fb_set(to_pci_dev(info->device), NULL);
+
 	fbcon_registered_fb[info->node] = NULL;
 	fbcon_num_registered_fb--;
 
@@ -3027,6 +3032,10 @@ static int do_fb_registered(struct fb_info *info)
 		}
 	}
 
+	/* Set the fb info for vga_switcheroo clients. Does nothing otherwise. */
+	if (info->device && dev_is_pci(info->device))
+		vga_switcheroo_client_fb_set(to_pci_dev(info->device), info);
+
 	return ret;
 }
 
-- 
2.51.1

Re: [PATCH] drm, fbcon, vga_switcheroo: Avoid race condition in fbcon setup

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Protect vga_switcheroo_client_fb_set() with console lock. Avoids OOB
> access in fbcon_remap_all(). Without holding the console lock the call
> races with switching outputs.
>
> VGA switcheroo calls fbcon_remap_all() when switching clients. The fbcon
> function uses struct fb_info.node, which is set by register_framebuffer().
> As the fb-helper code currently sets up VGA switcheroo before registering
> the framebuffer, the value of node is -1 and therefore not a legal value.
> For example, fbcon uses the value within set_con2fb_map() [1] as an index
> into an array.
>
> Moving vga_switcheroo_client_fb_set() after register_framebuffer() can
> result in VGA switching that does not switch fbcon correctly.
>
> Therefore move vga_switcheroo_client_fb_set() under fbcon_fb_registered(),
> which already holds the console lock. Fbdev calls fbcon_fb_registered()
> from within register_framebuffer(). Serializes the helper with VGA
> switcheroo's call to fbcon_remap_all().
>
> Although vga_switcheroo_client_fb_set() takes an instance of struct fb_info
> as parameter, it really only needs the contained fbcon state. Moving the
> call to fbcon initialization is therefore cleaner than before. Only amdgpu,
> i915, nouveau and radeon support vga_switcheroo. For all other drivers,
> this change does nothing.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://elixir.bootlin.com/linux/v6.17/source/drivers/video/fbdev/core/fbcon.c#L2942 # [1]
> ---

I'm not that familiar with fbcon and vga_switcheroo to properly review
your patch but after reading the explanation in the commit message and
reading the diff, the change does make sense to me.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

But I think that would be good if you get some testing for the drivers
that make use of vga_switcheroo. Also, do you need a Fixes tag ?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm, fbcon, vga_switcheroo: Avoid race condition in fbcon setup

[Thomas Zimmermann]

Hi

Am 17.11.25 um 11:32 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> Hello Thomas,
>
>> Protect vga_switcheroo_client_fb_set() with console lock. Avoids OOB
>> access in fbcon_remap_all(). Without holding the console lock the call
>> races with switching outputs.
>>
>> VGA switcheroo calls fbcon_remap_all() when switching clients. The fbcon
>> function uses struct fb_info.node, which is set by register_framebuffer().
>> As the fb-helper code currently sets up VGA switcheroo before registering
>> the framebuffer, the value of node is -1 and therefore not a legal value.
>> For example, fbcon uses the value within set_con2fb_map() [1] as an index
>> into an array.
>>
>> Moving vga_switcheroo_client_fb_set() after register_framebuffer() can
>> result in VGA switching that does not switch fbcon correctly.
>>
>> Therefore move vga_switcheroo_client_fb_set() under fbcon_fb_registered(),
>> which already holds the console lock. Fbdev calls fbcon_fb_registered()
>> from within register_framebuffer(). Serializes the helper with VGA
>> switcheroo's call to fbcon_remap_all().
>>
>> Although vga_switcheroo_client_fb_set() takes an instance of struct fb_info
>> as parameter, it really only needs the contained fbcon state. Moving the
>> call to fbcon initialization is therefore cleaner than before. Only amdgpu,
>> i915, nouveau and radeon support vga_switcheroo. For all other drivers,
>> this change does nothing.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Link: https://elixir.bootlin.com/linux/v6.17/source/drivers/video/fbdev/core/fbcon.c#L2942 # [1]
>> ---
> I'm not that familiar with fbcon and vga_switcheroo to properly review
> your patch but after reading the explanation in the commit message and
> reading the diff, the change does make sense to me.
>
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
>
> But I think that would be good if you get some testing for the drivers
> that make use of vga_switcheroo. Also, do you need a Fixes tag ?

I've ran the testing on amdgpu and i915 so that nothing breaks. The bug 
is hard to reproduce though. I've discovered it by reading the code.

About Fixes, the problem has been in the code forever. So IDK what Fixes 
would make sense. Just in case:

Fixes: 6a9ee8af344e ("vga_switcheroo: initial implementation (v15)")
Cc: Dave Airlie <airlied@redhat.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Helge Deller <deller@gmx.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
Cc: <stable@vger.kernel.org> # v2.6.34+


Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH] drm, fbcon, vga_switcheroo: Avoid race condition in fbcon setup

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 17.11.25 um 11:32 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>>
>> Hello Thomas,
>>
>>> Protect vga_switcheroo_client_fb_set() with console lock. Avoids OOB
>>> access in fbcon_remap_all(). Without holding the console lock the call
>>> races with switching outputs.
>>>
>>> VGA switcheroo calls fbcon_remap_all() when switching clients. The fbcon
>>> function uses struct fb_info.node, which is set by register_framebuffer().
>>> As the fb-helper code currently sets up VGA switcheroo before registering
>>> the framebuffer, the value of node is -1 and therefore not a legal value.
>>> For example, fbcon uses the value within set_con2fb_map() [1] as an index
>>> into an array.
>>>
>>> Moving vga_switcheroo_client_fb_set() after register_framebuffer() can
>>> result in VGA switching that does not switch fbcon correctly.
>>>
>>> Therefore move vga_switcheroo_client_fb_set() under fbcon_fb_registered(),
>>> which already holds the console lock. Fbdev calls fbcon_fb_registered()
>>> from within register_framebuffer(). Serializes the helper with VGA
>>> switcheroo's call to fbcon_remap_all().
>>>
>>> Although vga_switcheroo_client_fb_set() takes an instance of struct fb_info
>>> as parameter, it really only needs the contained fbcon state. Moving the
>>> call to fbcon initialization is therefore cleaner than before. Only amdgpu,
>>> i915, nouveau and radeon support vga_switcheroo. For all other drivers,
>>> this change does nothing.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Link: https://elixir.bootlin.com/linux/v6.17/source/drivers/video/fbdev/core/fbcon.c#L2942 # [1]
>>> ---
>> I'm not that familiar with fbcon and vga_switcheroo to properly review
>> your patch but after reading the explanation in the commit message and
>> reading the diff, the change does make sense to me.
>>
>> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> But I think that would be good if you get some testing for the drivers
>> that make use of vga_switcheroo. Also, do you need a Fixes tag ?
>
> I've ran the testing on amdgpu and i915 so that nothing breaks. The bug 
> is hard to reproduce though. I've discovered it by reading the code.
>

Thanks.

I usually put that kind of information between the --- separator and the
start of the diff. Since that info can be useful for reviewers and doesn't
end in the commited patch, due tools like `git am` omitting that section.

> About Fixes, the problem has been in the code forever. So IDK what Fixes 
> would make sense. Just in case:
>

I see. Then I agree that having the tag is less useful.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm, fbcon, vga_switcheroo: Avoid race condition in fbcon setup

[Alex Deucher]

On Wed, Nov 5, 2025 at 12:11 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Protect vga_switcheroo_client_fb_set() with console lock. Avoids OOB
> access in fbcon_remap_all(). Without holding the console lock the call
> races with switching outputs.
>
> VGA switcheroo calls fbcon_remap_all() when switching clients. The fbcon
> function uses struct fb_info.node, which is set by register_framebuffer().
> As the fb-helper code currently sets up VGA switcheroo before registering
> the framebuffer, the value of node is -1 and therefore not a legal value.
> For example, fbcon uses the value within set_con2fb_map() [1] as an index
> into an array.
>
> Moving vga_switcheroo_client_fb_set() after register_framebuffer() can
> result in VGA switching that does not switch fbcon correctly.
>
> Therefore move vga_switcheroo_client_fb_set() under fbcon_fb_registered(),
> which already holds the console lock. Fbdev calls fbcon_fb_registered()
> from within register_framebuffer(). Serializes the helper with VGA
> switcheroo's call to fbcon_remap_all().
>
> Although vga_switcheroo_client_fb_set() takes an instance of struct fb_info
> as parameter, it really only needs the contained fbcon state. Moving the
> call to fbcon initialization is therefore cleaner than before. Only amdgpu,
> i915, nouveau and radeon support vga_switcheroo. For all other drivers,
> this change does nothing.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://elixir.bootlin.com/linux/v6.17/source/drivers/video/fbdev/core/fbcon.c#L2942 # [1]

I haven't paged switcheroo into my head in a while, but this seems to
make sense so:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_fb_helper.c  | 14 --------------
>  drivers/video/fbdev/core/fbcon.c |  9 +++++++++
>  2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 53e9dc0543de..c0343ec16a57 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -31,9 +31,7 @@
>
>  #include <linux/console.h>
>  #include <linux/export.h>
> -#include <linux/pci.h>
>  #include <linux/sysrq.h>
> -#include <linux/vga_switcheroo.h>
>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_drv.h>
> @@ -570,11 +568,6 @@ EXPORT_SYMBOL(drm_fb_helper_release_info);
>   */
>  void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper)
>  {
> -       struct fb_info *info = fb_helper->info;
> -       struct device *dev = info->device;
> -
> -       if (dev_is_pci(dev))
> -               vga_switcheroo_client_fb_set(to_pci_dev(dev), NULL);
>         unregister_framebuffer(fb_helper->info);
>  }
>  EXPORT_SYMBOL(drm_fb_helper_unregister_info);
> @@ -1614,7 +1607,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
>         struct drm_client_dev *client = &fb_helper->client;
>         struct drm_device *dev = fb_helper->dev;
>         struct drm_fb_helper_surface_size sizes;
> -       struct fb_info *info;
>         int ret;
>
>         if (drm_WARN_ON(dev, !dev->driver->fbdev_probe))
> @@ -1635,12 +1627,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
>
>         strcpy(fb_helper->fb->comm, "[fbcon]");
>
> -       info = fb_helper->info;
> -
> -       /* Set the fb info for vgaswitcheroo clients. Does nothing otherwise. */
> -       if (dev_is_pci(info->device))
> -               vga_switcheroo_client_fb_set(to_pci_dev(info->device), info);
> -
>         return 0;
>  }
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 7f35ad66b462..863944530c8e 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -66,6 +66,7 @@
>  #include <linux/string.h>
>  #include <linux/kd.h>
>  #include <linux/panic.h>
> +#include <linux/pci.h>
>  #include <linux/printk.h>
>  #include <linux/slab.h>
>  #include <linux/fb.h>
> @@ -78,6 +79,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/crc32.h> /* For counting font checksums */
>  #include <linux/uaccess.h>
> +#include <linux/vga_switcheroo.h>
>  #include <asm/irq.h>
>
>  #include "fbcon.h"
> @@ -2894,6 +2896,9 @@ void fbcon_fb_unregistered(struct fb_info *info)
>
>         console_lock();
>
> +       if (info->device && dev_is_pci(info->device))
> +               vga_switcheroo_client_fb_set(to_pci_dev(info->device), NULL);
> +
>         fbcon_registered_fb[info->node] = NULL;
>         fbcon_num_registered_fb--;
>
> @@ -3027,6 +3032,10 @@ static int do_fb_registered(struct fb_info *info)
>                 }
>         }
>
> +       /* Set the fb info for vga_switcheroo clients. Does nothing otherwise. */
> +       if (info->device && dev_is_pci(info->device))
> +               vga_switcheroo_client_fb_set(to_pci_dev(info->device), info);
> +
>         return ret;
>  }
>
> --
> 2.51.1
>