[PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers

[PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers

[Javier Martinez Canillas]

Hello,

This series contains patches suggested by Daniel Vetter to fix a use-after-free
error in the fb_release() function, due a fb_info associated with a fbdev being
freed too early while a user-space process still has the fbdev dev node opened.

The is cuused by a wrong management of the struct fb_info lifetime in drivers,
but the fbdev core can also be made more resilient about it an leak

This can easily be reproduced with the simplefb driver doing the following:

$ cat < /dev/fb0 &
$ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind
$ kill %1

[  257.490471] ------------[ cut here ]------------
...
[  257.495125] refcount_t: underflow; use-after-free.
[  257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144
...
[  257.637482] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  257.644441] pc : refcount_warn_saturate+0xf4/0x144
[  257.649226] lr : refcount_warn_saturate+0xf4/0x144
[  257.654009] sp : ffff80000a06bbf0
[  257.657315] x29: ffff80000a06bbf0 x28: 000000000000000a x27: 000000000000000a
[  257.664448] x26: 0000000000000000 x25: ffff470b88c6a180 x24: 000000000000000a
[  257.671581] x23: ffff470b81706480 x22: ffff470b808c2160 x21: ffff470b8922ba20
[  257.678713] x20: ffff470b891f5810 x19: ffff470b891f5800 x18: ffffffffffffffff
[  257.685846] x17: 3a725f7463656a62 x16: ffffbb18c6465fd4 x15: 0720072007200720
[  257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375
[  257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : ffffbb18c58f6c90
[  257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0000000000000001
[  257.714373] x5 : ffff470bff8ec418 x4 : 0000000000000000 x3 : 0000000000000027
[  257.721506] x2 : 0000000000000000 x1 : 0000000000000027 x0 : 0000000000000026
[  257.728638] Call trace:
[  257.731075]  refcount_warn_saturate+0xf4/0x144
[  257.735513]  put_fb_info+0x70/0x7c
[  257.738916]  fb_release+0x60/0x74
[  257.742225]  __fput+0x88/0x240
[  257.745276]  ____fput+0x1c/0x30
[  257.748410]  task_work_run+0xc4/0x21c
[  257.752066]  do_exit+0x170/0x370
[  257.755288]  do_group_exit+0x40/0xb4
[  257.758858]  get_signal+0x8e0/0x90c
[  257.762339]  do_signal+0x1a0/0x280
[  257.765733]  do_notify_resume+0xc8/0x390
[  257.769650]  el0_da+0xe8/0xf0
[  257.772613]  el0t_64_sync_handler+0xe8/0x130
[  257.776877]  el0t_64_sync+0x190/0x194
[  257.780534] ---[ end trace 0000000000000000 ]---

Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free
to happen.

Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to
free the resources at the correct place.


Daniel Vetter (1):
  fbdev: Prevent possible use-after-free in fb_release()

Javier Martinez Canillas (2):
  fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
  fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove

 drivers/video/fbdev/core/fbsysfs.c | 4 ++++
 drivers/video/fbdev/efifb.c        | 9 ++++++++-
 drivers/video/fbdev/simplefb.c     | 8 +++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers

[Javier Martinez Canillas]

Hello,

This series contains patches suggested by Daniel Vetter to fix a use-after-free
error in the fb_release() function, due a fb_info associated with a fbdev being
freed too early while a user-space process still has the fbdev dev node opened.

The is cuused by a wrong management of the struct fb_info lifetime in drivers,
but the fbdev core can also be made more resilient about it an leak

This can easily be reproduced with the simplefb driver doing the following:

$ cat < /dev/fb0 &
$ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind
$ kill %1

[  257.490471] ------------[ cut here ]------------
...
[  257.495125] refcount_t: underflow; use-after-free.
[  257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144
...
[  257.637482] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  257.644441] pc : refcount_warn_saturate+0xf4/0x144
[  257.649226] lr : refcount_warn_saturate+0xf4/0x144
[  257.654009] sp : ffff80000a06bbf0
[  257.657315] x29: ffff80000a06bbf0 x28: 000000000000000a x27: 000000000000000a
[  257.664448] x26: 0000000000000000 x25: ffff470b88c6a180 x24: 000000000000000a
[  257.671581] x23: ffff470b81706480 x22: ffff470b808c2160 x21: ffff470b8922ba20
[  257.678713] x20: ffff470b891f5810 x19: ffff470b891f5800 x18: ffffffffffffffff
[  257.685846] x17: 3a725f7463656a62 x16: ffffbb18c6465fd4 x15: 0720072007200720
[  257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375
[  257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : ffffbb18c58f6c90
[  257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0000000000000001
[  257.714373] x5 : ffff470bff8ec418 x4 : 0000000000000000 x3 : 0000000000000027
[  257.721506] x2 : 0000000000000000 x1 : 0000000000000027 x0 : 0000000000000026
[  257.728638] Call trace:
[  257.731075]  refcount_warn_saturate+0xf4/0x144
[  257.735513]  put_fb_info+0x70/0x7c
[  257.738916]  fb_release+0x60/0x74
[  257.742225]  __fput+0x88/0x240
[  257.745276]  ____fput+0x1c/0x30
[  257.748410]  task_work_run+0xc4/0x21c
[  257.752066]  do_exit+0x170/0x370
[  257.755288]  do_group_exit+0x40/0xb4
[  257.758858]  get_signal+0x8e0/0x90c
[  257.762339]  do_signal+0x1a0/0x280
[  257.765733]  do_notify_resume+0xc8/0x390
[  257.769650]  el0_da+0xe8/0xf0
[  257.772613]  el0t_64_sync_handler+0xe8/0x130
[  257.776877]  el0t_64_sync+0x190/0x194
[  257.780534] ---[ end trace 0000000000000000 ]---

Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free
to happen.

Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to
free the resources at the correct place.


Daniel Vetter (1):
  fbdev: Prevent possible use-after-free in fb_release()

Javier Martinez Canillas (2):
  fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
  fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove

 drivers/video/fbdev/core/fbsysfs.c | 4 ++++
 drivers/video/fbdev/efifb.c        | 9 ++++++++-
 drivers/video/fbdev/simplefb.c     | 8 +++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 1/3] fbdev: Prevent possible use-after-free in fb_release()

[Javier Martinez Canillas]

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>
---

 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 26892940c213..82e31a2d845e 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;
+
 	kfree(info->apertures);
 	kfree(info);
 }
-- 
2.35.1

[PATCH 1/3] fbdev: Prevent possible use-after-free in fb_release()

[Javier Martinez Canillas]

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>
---

 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 26892940c213..82e31a2d845e 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;
+
 	kfree(info->apertures);
 	kfree(info);
 }
-- 
2.35.1

[PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.

This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.

The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/video/fbdev/simplefb.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..2c198561c338 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -84,6 +84,10 @@ struct simplefb_par {
 static void simplefb_clocks_destroy(struct simplefb_par *par);
 static void simplefb_regulators_destroy(struct simplefb_par *par);
 
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
 static void simplefb_destroy(struct fb_info *info)
 {
 	struct simplefb_par *par = info->par;
@@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
 	if (info->screen_base)
 		iounmap(info->screen_base);
 
+	framebuffer_release(info);
+
 	if (mem)
 		release_mem_region(mem->start, resource_size(mem));
 }
@@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
 
+	/* simplefb_destroy takes care of info cleanup */
 	unregister_framebuffer(info);
-	framebuffer_release(info);
 
 	return 0;
 }
-- 
2.35.1

[PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.

This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.

The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/video/fbdev/simplefb.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..2c198561c338 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -84,6 +84,10 @@ struct simplefb_par {
 static void simplefb_clocks_destroy(struct simplefb_par *par);
 static void simplefb_regulators_destroy(struct simplefb_par *par);
 
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
 static void simplefb_destroy(struct fb_info *info)
 {
 	struct simplefb_par *par = info->par;
@@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
 	if (info->screen_base)
 		iounmap(info->screen_base);
 
+	framebuffer_release(info);
+
 	if (mem)
 		release_mem_region(mem->start, resource_size(mem));
 }
@@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
 
+	/* simplefb_destroy takes care of info cleanup */
 	unregister_framebuffer(info);
-	framebuffer_release(info);
 
 	return 0;
 }
-- 
2.35.1

[PATCH 3/3] fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.

This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.

The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/video/fbdev/efifb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..cfa3dc0b4eee 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
 static inline void efifb_show_boot_graphics(struct fb_info *info) {}
 #endif
 
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
 static void efifb_destroy(struct fb_info *info)
 {
 	if (efifb_pci_dev)
@@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
 		else
 			memunmap(info->screen_base);
 	}
+
+	framebuffer_release(info);
+
 	if (request_mem_succeeded)
 		release_mem_region(info->apertures->ranges[0].base,
 				   info->apertures->ranges[0].size);
@@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
 
+	/* efifb_destroy takes care of info cleanup */
 	unregister_framebuffer(info);
 	sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
-	framebuffer_release(info);
 
 	return 0;
 }
-- 
2.35.1

[PATCH 3/3] fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

The driver is calling framebuffer_release() in its .remove callback, but
this will cause the struct fb_info to be freed too early. Since it could
be that a reference is still hold to it if user-space opened the fbdev.

This would lead to a use-after-free error if the framebuffer device was
unregistered but later a user-space process tries to close the fbdev fd.

The correct thing to do is to only unregister the framebuffer in the
driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/video/fbdev/efifb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..cfa3dc0b4eee 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
 static inline void efifb_show_boot_graphics(struct fb_info *info) {}
 #endif
 
+/*
+ * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
+ * of unregister_framebuffer() or fb_release(). Do any cleanup here.
+ */
 static void efifb_destroy(struct fb_info *info)
 {
 	if (efifb_pci_dev)
@@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
 		else
 			memunmap(info->screen_base);
 	}
+
+	framebuffer_release(info);
+
 	if (request_mem_succeeded)
 		release_mem_region(info->apertures->ranges[0].base,
 				   info->apertures->ranges[0].size);
@@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
 
+	/* efifb_destroy takes care of info cleanup */
 	unregister_framebuffer(info);
 	sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
-	framebuffer_release(info);
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2700 bytes --]

Hi

Am 04.05.22 um 23:57 schrieb Javier Martinez Canillas:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
> 
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
> 
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Please see my question below.

> ---
> 
>   drivers/video/fbdev/simplefb.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 94fc9c6d0411..2c198561c338 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -84,6 +84,10 @@ struct simplefb_par {
>   static void simplefb_clocks_destroy(struct simplefb_par *par);
>   static void simplefb_regulators_destroy(struct simplefb_par *par);
>   
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
>   static void simplefb_destroy(struct fb_info *info)
>   {
>   	struct simplefb_par *par = info->par;
> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>   	if (info->screen_base)
>   		iounmap(info->screen_base);
>   
> +	framebuffer_release(info);
> +
>   	if (mem)
>   		release_mem_region(mem->start, resource_size(mem));

The original problem with fbdev hot-unplug was that vmwgfx needed the 
framebuffer region to be released. If we release it only after userspace 
closed it's final file descriptor, vmwgfx could have already failed.

I still don't fully get why this code apparently works or at least 
doesn't blow up occasionally. Any ideas?

Best regards
Thomas

>   }
> @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
>   {
>   	struct fb_info *info = platform_get_drvdata(pdev);
>   
> +	/* simplefb_destroy takes care of info cleanup */
>   	unregister_framebuffer(info);
> -	framebuffer_release(info);
>   
>   	return 0;
>   }

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2700 bytes --]

Hi

Am 04.05.22 um 23:57 schrieb Javier Martinez Canillas:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
> 
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
> 
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Please see my question below.

> ---
> 
>   drivers/video/fbdev/simplefb.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 94fc9c6d0411..2c198561c338 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -84,6 +84,10 @@ struct simplefb_par {
>   static void simplefb_clocks_destroy(struct simplefb_par *par);
>   static void simplefb_regulators_destroy(struct simplefb_par *par);
>   
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
>   static void simplefb_destroy(struct fb_info *info)
>   {
>   	struct simplefb_par *par = info->par;
> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>   	if (info->screen_base)
>   		iounmap(info->screen_base);
>   
> +	framebuffer_release(info);
> +
>   	if (mem)
>   		release_mem_region(mem->start, resource_size(mem));

The original problem with fbdev hot-unplug was that vmwgfx needed the 
framebuffer region to be released. If we release it only after userspace 
closed it's final file descriptor, vmwgfx could have already failed.

I still don't fully get why this code apparently works or at least 
doesn't blow up occasionally. Any ideas?

Best regards
Thomas

>   }
> @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
>   {
>   	struct fb_info *info = platform_get_drvdata(pdev);
>   
> +	/* simplefb_destroy takes care of info cleanup */
>   	unregister_framebuffer(info);
> -	framebuffer_release(info);
>   
>   	return 0;
>   }

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/3] fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2356 bytes --]



Am 04.05.22 um 23:58 schrieb Javier Martinez Canillas:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
> 
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
> 
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
> 
>   drivers/video/fbdev/efifb.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ea42ba6445b2..cfa3dc0b4eee 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
>   static inline void efifb_show_boot_graphics(struct fb_info *info) {}
>   #endif
>   
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
>   static void efifb_destroy(struct fb_info *info)
>   {
>   	if (efifb_pci_dev)
> @@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
>   		else
>   			memunmap(info->screen_base);
>   	}
> +
> +	framebuffer_release(info);
> +
>   	if (request_mem_succeeded)
>   		release_mem_region(info->apertures->ranges[0].base,
>   				   info->apertures->ranges[0].size);
> @@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
>   {
>   	struct fb_info *info = platform_get_drvdata(pdev);
>   
> +	/* efifb_destroy takes care of info cleanup */
>   	unregister_framebuffer(info);
>   	sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
> -	framebuffer_release(info);
>   
>   	return 0;
>   }

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/3] fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2356 bytes --]



Am 04.05.22 um 23:58 schrieb Javier Martinez Canillas:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
> 
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
> 
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
> 
>   drivers/video/fbdev/efifb.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ea42ba6445b2..cfa3dc0b4eee 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info)
>   static inline void efifb_show_boot_graphics(struct fb_info *info) {}
>   #endif
>   
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
>   static void efifb_destroy(struct fb_info *info)
>   {
>   	if (efifb_pci_dev)
> @@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info)
>   		else
>   			memunmap(info->screen_base);
>   	}
> +
> +	framebuffer_release(info);
> +
>   	if (request_mem_succeeded)
>   		release_mem_region(info->apertures->ranges[0].base,
>   				   info->apertures->ranges[0].size);
> @@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev)
>   {
>   	struct fb_info *info = platform_get_drvdata(pdev);
>   
> +	/* efifb_destroy takes care of info cleanup */
>   	unregister_framebuffer(info);
>   	sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
> -	framebuffer_release(info);
>   
>   	return 0;
>   }

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/3] fbdev: Prevent possible use-after-free in fb_release()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1566 bytes --]



Am 04.05.22 um 23:56 schrieb Javier Martinez Canillas:
> 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>

> ---
> 
>   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 26892940c213..82e31a2d845e 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;
> +
>   	kfree(info->apertures);
>   	kfree(info);
>   }

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/3] fbdev: Prevent possible use-after-free in fb_release()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1566 bytes --]



Am 04.05.22 um 23:56 schrieb Javier Martinez Canillas:
> 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>

> ---
> 
>   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 26892940c213..82e31a2d845e 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;
> +
>   	kfree(info->apertures);
>   	kfree(info);
>   }

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

Hello Thomas,

On 5/5/22 09:29, Thomas Zimmermann wrote:

[snip]

>>   static void simplefb_destroy(struct fb_info *info)
>>   {
>>   	struct simplefb_par *par = info->par;
>> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>>   	if (info->screen_base)
>>   		iounmap(info->screen_base);
>>   
>> +	framebuffer_release(info);
>> +
>>   	if (mem)
>>   		release_mem_region(mem->start, resource_size(mem));
> 
> The original problem with fbdev hot-unplug was that vmwgfx needed the 
> framebuffer region to be released. If we release it only after userspace 
> closed it's final file descriptor, vmwgfx could have already failed.
> 
> I still don't fully get why this code apparently works or at least 
> doesn't blow up occasionally. Any ideas?
>

I believe that vmwgfx doesn't fail to probe (or any other DRM driver)
only when there are not user-space processes with a fbdev node opened
since otherwise as you said the memory wouldn't be released yet.

unregister_framebuffer() is called from the driver's .remove handler
and that decrement the fb_info refcount, so if reaches zero it will
call to the fb fops .destroy() handler and release the I/O memory.

In other words, in most cases (i.e: only fbcon bound to the fbdev)
the driver's removal/ device unbind and the memory release will be
at the same time.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

Hello Thomas,

On 5/5/22 09:29, Thomas Zimmermann wrote:

[snip]

>>   static void simplefb_destroy(struct fb_info *info)
>>   {
>>   	struct simplefb_par *par = info->par;
>> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>>   	if (info->screen_base)
>>   		iounmap(info->screen_base);
>>   
>> +	framebuffer_release(info);
>> +
>>   	if (mem)
>>   		release_mem_region(mem->start, resource_size(mem));
> 
> The original problem with fbdev hot-unplug was that vmwgfx needed the 
> framebuffer region to be released. If we release it only after userspace 
> closed it's final file descriptor, vmwgfx could have already failed.
> 
> I still don't fully get why this code apparently works or at least 
> doesn't blow up occasionally. Any ideas?
>

I believe that vmwgfx doesn't fail to probe (or any other DRM driver)
only when there are not user-space processes with a fbdev node opened
since otherwise as you said the memory wouldn't be released yet.

unregister_framebuffer() is called from the driver's .remove handler
and that decrement the fb_info refcount, so if reaches zero it will
call to the fb fops .destroy() handler and release the I/O memory.

In other words, in most cases (i.e: only fbcon bound to the fbdev)
the driver's removal/ device unbind and the memory release will be
at the same time.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2308 bytes --]

Hi

Am 05.05.22 um 09:38 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 5/5/22 09:29, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>>    static void simplefb_destroy(struct fb_info *info)
>>>    {
>>>    	struct simplefb_par *par = info->par;
>>> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>>>    	if (info->screen_base)
>>>    		iounmap(info->screen_base);
>>>    
>>> +	framebuffer_release(info);
>>> +
>>>    	if (mem)
>>>    		release_mem_region(mem->start, resource_size(mem));
>>
>> The original problem with fbdev hot-unplug was that vmwgfx needed the
>> framebuffer region to be released. If we release it only after userspace
>> closed it's final file descriptor, vmwgfx could have already failed.
>>
>> I still don't fully get why this code apparently works or at least
>> doesn't blow up occasionally. Any ideas?
>>
> 
> I believe that vmwgfx doesn't fail to probe (or any other DRM driver)
> only when there are not user-space processes with a fbdev node opened
> since otherwise as you said the memory wouldn't be released yet.
> 
> unregister_framebuffer() is called from the driver's .remove handler
> and that decrement the fb_info refcount, so if reaches zero it will
> call to the fb fops .destroy() handler and release the I/O memory.
> 
> In other words, in most cases (i.e: only fbcon bound to the fbdev)
> the driver's removal/ device unbind and the memory release will be
> at the same time.
> 

We're one the same page here, but it's still sort of a mystery to me why 
this works in practice.

I'm specifically talking about pci_request_regions() in vmwgfx [1]. IIRC 
this would fail if simplefb still owns the framebuffer region. Lots of 
systems run Plymouth during boot and this should result in failures 
occasionally. Still, we never heard about anything.

Of course, it's always been broken (even long before real fbdev 
hotunplugging). Switching to simpledrm resolves the problem.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.17.5/source/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c#L727

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3668 bytes --]

Hi

Am 04.05.22 um 23:51 schrieb Javier Martinez Canillas:
> Hello,
> 
> This series contains patches suggested by Daniel Vetter to fix a use-after-free
> error in the fb_release() function, due a fb_info associated with a fbdev being
> freed too early while a user-space process still has the fbdev dev node opened.
> 
> The is cuused by a wrong management of the struct fb_info lifetime in drivers,
> but the fbdev core can also be made more resilient about it an leak
> 
> This can easily be reproduced with the simplefb driver doing the following:
> 
> $ cat < /dev/fb0 &
> $ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind
> $ kill %1
> 
> [  257.490471] ------------[ cut here ]------------
> ...
> [  257.495125] refcount_t: underflow; use-after-free.
> [  257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144
> ...
> [  257.637482] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  257.644441] pc : refcount_warn_saturate+0xf4/0x144
> [  257.649226] lr : refcount_warn_saturate+0xf4/0x144
> [  257.654009] sp : ffff80000a06bbf0
> [  257.657315] x29: ffff80000a06bbf0 x28: 000000000000000a x27: 000000000000000a
> [  257.664448] x26: 0000000000000000 x25: ffff470b88c6a180 x24: 000000000000000a
> [  257.671581] x23: ffff470b81706480 x22: ffff470b808c2160 x21: ffff470b8922ba20
> [  257.678713] x20: ffff470b891f5810 x19: ffff470b891f5800 x18: ffffffffffffffff
> [  257.685846] x17: 3a725f7463656a62 x16: ffffbb18c6465fd4 x15: 0720072007200720
> [  257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375
> [  257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : ffffbb18c58f6c90
> [  257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0000000000000001
> [  257.714373] x5 : ffff470bff8ec418 x4 : 0000000000000000 x3 : 0000000000000027
> [  257.721506] x2 : 0000000000000000 x1 : 0000000000000027 x0 : 0000000000000026
> [  257.728638] Call trace:
> [  257.731075]  refcount_warn_saturate+0xf4/0x144
> [  257.735513]  put_fb_info+0x70/0x7c
> [  257.738916]  fb_release+0x60/0x74
> [  257.742225]  __fput+0x88/0x240
> [  257.745276]  ____fput+0x1c/0x30
> [  257.748410]  task_work_run+0xc4/0x21c
> [  257.752066]  do_exit+0x170/0x370
> [  257.755288]  do_group_exit+0x40/0xb4
> [  257.758858]  get_signal+0x8e0/0x90c
> [  257.762339]  do_signal+0x1a0/0x280
> [  257.765733]  do_notify_resume+0xc8/0x390
> [  257.769650]  el0_da+0xe8/0xf0
> [  257.772613]  el0t_64_sync_handler+0xe8/0x130
> [  257.776877]  el0t_64_sync+0x190/0x194
> [  257.780534] ---[ end trace 0000000000000000 ]---
> 
> Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free
> to happen.
> 
> Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to
> free the resources at the correct place.

 From a quick look, vesafb seems to be affected as well.

Best regards
Thomas

> 
> 
> Daniel Vetter (1):
>    fbdev: Prevent possible use-after-free in fb_release()
> 
> Javier Martinez Canillas (2):
>    fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
>    fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove
> 
>   drivers/video/fbdev/core/fbsysfs.c | 4 ++++
>   drivers/video/fbdev/efifb.c        | 9 ++++++++-
>   drivers/video/fbdev/simplefb.c     | 8 +++++++-
>   3 files changed, 19 insertions(+), 2 deletions(-)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3668 bytes --]

Hi

Am 04.05.22 um 23:51 schrieb Javier Martinez Canillas:
> Hello,
> 
> This series contains patches suggested by Daniel Vetter to fix a use-after-free
> error in the fb_release() function, due a fb_info associated with a fbdev being
> freed too early while a user-space process still has the fbdev dev node opened.
> 
> The is cuused by a wrong management of the struct fb_info lifetime in drivers,
> but the fbdev core can also be made more resilient about it an leak
> 
> This can easily be reproduced with the simplefb driver doing the following:
> 
> $ cat < /dev/fb0 &
> $ echo simple-framebuffer.0 > /sys/bus/platform/drivers/simple-framebuffer/unbind
> $ kill %1
> 
> [  257.490471] ------------[ cut here ]------------
> ...
> [  257.495125] refcount_t: underflow; use-after-free.
> [  257.495222] WARNING: CPU: 0 PID: 975 at lib/refcount.c:28 refcount_warn_saturate+0xf4/0x144
> ...
> [  257.637482] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  257.644441] pc : refcount_warn_saturate+0xf4/0x144
> [  257.649226] lr : refcount_warn_saturate+0xf4/0x144
> [  257.654009] sp : ffff80000a06bbf0
> [  257.657315] x29: ffff80000a06bbf0 x28: 000000000000000a x27: 000000000000000a
> [  257.664448] x26: 0000000000000000 x25: ffff470b88c6a180 x24: 000000000000000a
> [  257.671581] x23: ffff470b81706480 x22: ffff470b808c2160 x21: ffff470b8922ba20
> [  257.678713] x20: ffff470b891f5810 x19: ffff470b891f5800 x18: ffffffffffffffff
> [  257.685846] x17: 3a725f7463656a62 x16: ffffbb18c6465fd4 x15: 0720072007200720
> [  257.692978] x14: 0720072d072d072d x13: 0a2e656572662d72 x12: 657466612d657375
> [  257.700110] x11: 203b776f6c667265 x10: 646e75203a745f74 x9 : ffffbb18c58f6c90
> [  257.707242] x8 : 75203b776f6c6672 x7 : 65646e75203a745f x6 : 0000000000000001
> [  257.714373] x5 : ffff470bff8ec418 x4 : 0000000000000000 x3 : 0000000000000027
> [  257.721506] x2 : 0000000000000000 x1 : 0000000000000027 x0 : 0000000000000026
> [  257.728638] Call trace:
> [  257.731075]  refcount_warn_saturate+0xf4/0x144
> [  257.735513]  put_fb_info+0x70/0x7c
> [  257.738916]  fb_release+0x60/0x74
> [  257.742225]  __fput+0x88/0x240
> [  257.745276]  ____fput+0x1c/0x30
> [  257.748410]  task_work_run+0xc4/0x21c
> [  257.752066]  do_exit+0x170/0x370
> [  257.755288]  do_group_exit+0x40/0xb4
> [  257.758858]  get_signal+0x8e0/0x90c
> [  257.762339]  do_signal+0x1a0/0x280
> [  257.765733]  do_notify_resume+0xc8/0x390
> [  257.769650]  el0_da+0xe8/0xf0
> [  257.772613]  el0t_64_sync_handler+0xe8/0x130
> [  257.776877]  el0t_64_sync+0x190/0x194
> [  257.780534] ---[ end trace 0000000000000000 ]---
> 
> Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free
> to happen.
> 
> Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to
> free the resources at the correct place.

 From a quick look, vesafb seems to be affected as well.

Best regards
Thomas

> 
> 
> Daniel Vetter (1):
>    fbdev: Prevent possible use-after-free in fb_release()
> 
> Javier Martinez Canillas (2):
>    fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove
>    fbdev/efifb: Cleanup fb_info in .fb_destroy rather than .remove
> 
>   drivers/video/fbdev/core/fbsysfs.c | 4 ++++
>   drivers/video/fbdev/efifb.c        | 9 ++++++++-
>   drivers/video/fbdev/simplefb.c     | 8 +++++++-
>   3 files changed, 19 insertions(+), 2 deletions(-)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

Hello Thomas,

On 5/5/22 10:05, Thomas Zimmermann wrote:

[snip]

>>
>> In other words, in most cases (i.e: only fbcon bound to the fbdev)
>> the driver's removal/ device unbind and the memory release will be
>> at the same time.
>>
> 
> We're one the same page here, but it's still sort of a mystery to me why 
> this works in practice.
> 
> I'm specifically talking about pci_request_regions() in vmwgfx [1]. IIRC 
> this would fail if simplefb still owns the framebuffer region. Lots of 
> systems run Plymouth during boot and this should result in failures 
> occasionally. Still, we never heard about anything.
>

Yes, I think is because Plymouth IIUC waits for a /dev/dri/card? to be
present and only uses a /dev/fb? as a fallback if a timeout expires.

At least in Fedora (even before the efifb -> simpledrm change) it will
use KMS/DRM since the DRM kernel module for the graphics device in the
machine would be in the intird.

So efifb was only used for fbcon and plymouth would only use DRM/KMS
and not its fbdev backend.

This seems to be sort of a corner case when you have {efi,simple}fb
in the early boot but the real DRM module only in the rootfs after the
initrd has done a pivot_root(2).
 
> Of course, it's always been broken (even long before real fbdev 
> hotunplugging). Switching to simpledrm resolves the problem.
>

Indeed. My opinion after dealing with these fbdev problems is that we
shouldn't try to fix all possible corner cases and just try to get rid
of fbdev as soon as possible.
 -- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers

[Javier Martinez Canillas]

Hello Thomas,

On 5/5/22 10:16, Thomas Zimmermann wrote:

[snip]

>> Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free
>> to happen.
>>
>> Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to
>> free the resources at the correct place.
> 
>  From a quick look, vesafb seems to be affected as well.
>

Right, I wrongly assumed that we only cared about efifb and simplefb but forgot
that vesafb is used when setting a VESA mode with vga=foo. I'll add it in a v2.
 
> Best regards
> Thomas
> 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/3] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers

[Javier Martinez Canillas]

Hello Thomas,

On 5/5/22 10:16, Thomas Zimmermann wrote:

[snip]

>> Patch #1 adds a WARN_ON() to framebuffer_release() to prevent the use-after-free
>> to happen.
>>
>> Patch #2 and patch #3 fixes the simplefb and efifb drivers respectively, to
>> free the resources at the correct place.
> 
>  From a quick look, vesafb seems to be affected as well.
>

Right, I wrongly assumed that we only cared about efifb and simplefb but forgot
that vesafb is used when setting a VESA mode with vga=foo. I'll add it in a v2.
 
> Best regards
> Thomas
> 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2373 bytes --]

Hi

Am 05.05.22 um 10:28 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 5/5/22 10:05, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>>
>>> In other words, in most cases (i.e: only fbcon bound to the fbdev)
>>> the driver's removal/ device unbind and the memory release will be
>>> at the same time.
>>>
>>
>> We're one the same page here, but it's still sort of a mystery to me why
>> this works in practice.
>>
>> I'm specifically talking about pci_request_regions() in vmwgfx [1]. IIRC
>> this would fail if simplefb still owns the framebuffer region. Lots of
>> systems run Plymouth during boot and this should result in failures
>> occasionally. Still, we never heard about anything.
>>
> 
> Yes, I think is because Plymouth IIUC waits for a /dev/dri/card? to be
> present and only uses a /dev/fb? as a fallback if a timeout expires.

Oh, right! The infamous plymouth timeout. 'sleep(30)' is the swiss-army 
knife of concurrent programming. ;)

But I'm not blaming anyone. There are situations where nothing else 
helps. Plymouth really can't do anything else here. We've received 
reports for gfx-handover bugs when the timeout expired and plymouth uses 
the fbdev. The system got stuck then because of fbdev IIRC.

Best regards
Thomas

> 
> At least in Fedora (even before the efifb -> simpledrm change) it will
> use KMS/DRM since the DRM kernel module for the graphics device in the
> machine would be in the intird.
> 
> So efifb was only used for fbcon and plymouth would only use DRM/KMS
> and not its fbdev backend.
> 
> This seems to be sort of a corner case when you have {efi,simple}fb
> in the early boot but the real DRM module only in the rootfs after the
> initrd has done a pivot_root(2).
>   
>> Of course, it's always been broken (even long before real fbdev
>> hotunplugging). Switching to simpledrm resolves the problem.
>>
> 
> Indeed. My opinion after dealing with these fbdev problems is that we
> shouldn't try to fix all possible corner cases and just try to get rid
> of fbdev as soon as possible.
>   --
> Best regards,
> 
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Daniel Vetter]

On Wed, May 04, 2022 at 11:57:22PM +0200, Javier Martinez Canillas wrote:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
> 
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
> 
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

I think this should have a Fixes: line for the patch from Thomas which
changed the remove_conflicting_fb code:

27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")

I think we should also mention that strictly speaking the code flow is now
wrong, because hw cleanup (like iounmap) should be done from ->remove
while sw cleanup (like calling framebuffer_release()) is the only thing
that should be done from ->fb_destroy. But the current code matches what
was happening before 27599aacbaef so more minimal "fix"

With those details added Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Same for the next patch.
-Daniel

> ---
> 
>  drivers/video/fbdev/simplefb.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 94fc9c6d0411..2c198561c338 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -84,6 +84,10 @@ struct simplefb_par {
>  static void simplefb_clocks_destroy(struct simplefb_par *par);
>  static void simplefb_regulators_destroy(struct simplefb_par *par);
>  
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
>  static void simplefb_destroy(struct fb_info *info)
>  {
>  	struct simplefb_par *par = info->par;
> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>  	if (info->screen_base)
>  		iounmap(info->screen_base);
>  
> +	framebuffer_release(info);
> +
>  	if (mem)
>  		release_mem_region(mem->start, resource_size(mem));
>  }
> @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
>  {
>  	struct fb_info *info = platform_get_drvdata(pdev);
>  
> +	/* simplefb_destroy takes care of info cleanup */
>  	unregister_framebuffer(info);
> -	framebuffer_release(info);
>  
>  	return 0;
>  }
> -- 
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Daniel Vetter]

On Wed, May 04, 2022 at 11:57:22PM +0200, Javier Martinez Canillas wrote:
> The driver is calling framebuffer_release() in its .remove callback, but
> this will cause the struct fb_info to be freed too early. Since it could
> be that a reference is still hold to it if user-space opened the fbdev.
> 
> This would lead to a use-after-free error if the framebuffer device was
> unregistered but later a user-space process tries to close the fbdev fd.
> 
> The correct thing to do is to only unregister the framebuffer in the
> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

I think this should have a Fixes: line for the patch from Thomas which
changed the remove_conflicting_fb code:

27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")

I think we should also mention that strictly speaking the code flow is now
wrong, because hw cleanup (like iounmap) should be done from ->remove
while sw cleanup (like calling framebuffer_release()) is the only thing
that should be done from ->fb_destroy. But the current code matches what
was happening before 27599aacbaef so more minimal "fix"

With those details added Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Same for the next patch.
-Daniel

> ---
> 
>  drivers/video/fbdev/simplefb.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 94fc9c6d0411..2c198561c338 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -84,6 +84,10 @@ struct simplefb_par {
>  static void simplefb_clocks_destroy(struct simplefb_par *par);
>  static void simplefb_regulators_destroy(struct simplefb_par *par);
>  
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> + */
>  static void simplefb_destroy(struct fb_info *info)
>  {
>  	struct simplefb_par *par = info->par;
> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>  	if (info->screen_base)
>  		iounmap(info->screen_base);
>  
> +	framebuffer_release(info);
> +
>  	if (mem)
>  		release_mem_region(mem->start, resource_size(mem));
>  }
> @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
>  {
>  	struct fb_info *info = platform_get_drvdata(pdev);
>  
> +	/* simplefb_destroy takes care of info cleanup */
>  	unregister_framebuffer(info);
> -	framebuffer_release(info);
>  
>  	return 0;
>  }
> -- 
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Javier Martinez Canillas]

Hello Daniel,

On 5/5/22 14:52, Daniel Vetter wrote:
> On Wed, May 04, 2022 at 11:57:22PM +0200, Javier Martinez Canillas wrote:
>> The driver is calling framebuffer_release() in its .remove callback, but
>> this will cause the struct fb_info to be freed too early. Since it could
>> be that a reference is still hold to it if user-space opened the fbdev.
>>
>> This would lead to a use-after-free error if the framebuffer device was
>> unregistered but later a user-space process tries to close the fbdev fd.
>>
>> The correct thing to do is to only unregister the framebuffer in the
>> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
>>
>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> I think this should have a Fixes: line for the patch from Thomas which
> changed the remove_conflicting_fb code:
> 
> 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
>

Ok.
 
> I think we should also mention that strictly speaking the code flow is now
> wrong, because hw cleanup (like iounmap) should be done from ->remove
> while sw cleanup (like calling framebuffer_release()) is the only thing
> that should be done from ->fb_destroy. But the current code matches what
> was happening before 27599aacbaef so more minimal "fix"
>

Yes, I tried to make it as small as possible. Thomas pointed out that vesafb
has the same issue and I included in v2. I had move things around more there
though.
 
> With those details added Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Same for the next patch.

Thanks. I'll post a v3 adding what you suggested but probably not today.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Daniel Vetter]

On Thu, May 05, 2022 at 09:29:40AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 04.05.22 um 23:57 schrieb Javier Martinez Canillas:
> > The driver is calling framebuffer_release() in its .remove callback, but
> > this will cause the struct fb_info to be freed too early. Since it could
> > be that a reference is still hold to it if user-space opened the fbdev.
> > 
> > This would lead to a use-after-free error if the framebuffer device was
> > unregistered but later a user-space process tries to close the fbdev fd.
> > 
> > The correct thing to do is to only unregister the framebuffer in the
> > driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Please see my question below.
> 
> > ---
> > 
> >   drivers/video/fbdev/simplefb.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> > index 94fc9c6d0411..2c198561c338 100644
> > --- a/drivers/video/fbdev/simplefb.c
> > +++ b/drivers/video/fbdev/simplefb.c
> > @@ -84,6 +84,10 @@ struct simplefb_par {
> >   static void simplefb_clocks_destroy(struct simplefb_par *par);
> >   static void simplefb_regulators_destroy(struct simplefb_par *par);
> > +/*
> > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> > + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> > + */
> >   static void simplefb_destroy(struct fb_info *info)
> >   {
> >   	struct simplefb_par *par = info->par;
> > @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
> >   	if (info->screen_base)
> >   		iounmap(info->screen_base);
> > +	framebuffer_release(info);
> > +
> >   	if (mem)
> >   		release_mem_region(mem->start, resource_size(mem));
> 
> The original problem with fbdev hot-unplug was that vmwgfx needed the
> framebuffer region to be released. If we release it only after userspace
> closed it's final file descriptor, vmwgfx could have already failed.
> 
> I still don't fully get why this code apparently works or at least doesn't
> blow up occasionally. Any ideas?

See my other reply, releasing hw stuff should be done from ->remove, not
->fb_destroy.

Also note that none of the patches discussed moved around anything here,
we simply leaked things a bit when only unregistering the fb and not going
through the device->remove callback. I guess in practice it works because
unregistering the device sends out a uevent, and userspace then closes all
it's device fd as a reaction to that, and usually that's fast enough to
not upset anyone?

No idea tbh.
-Daniel

> Best regards
> Thomas
> 
> >   }
> > @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
> >   {
> >   	struct fb_info *info = platform_get_drvdata(pdev);
> > +	/* simplefb_destroy takes care of info cleanup */
> >   	unregister_framebuffer(info);
> > -	framebuffer_release(info);
> >   	return 0;
> >   }
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Daniel Vetter]

On Thu, May 05, 2022 at 09:29:40AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 04.05.22 um 23:57 schrieb Javier Martinez Canillas:
> > The driver is calling framebuffer_release() in its .remove callback, but
> > this will cause the struct fb_info to be freed too early. Since it could
> > be that a reference is still hold to it if user-space opened the fbdev.
> > 
> > This would lead to a use-after-free error if the framebuffer device was
> > unregistered but later a user-space process tries to close the fbdev fd.
> > 
> > The correct thing to do is to only unregister the framebuffer in the
> > driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Please see my question below.
> 
> > ---
> > 
> >   drivers/video/fbdev/simplefb.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> > index 94fc9c6d0411..2c198561c338 100644
> > --- a/drivers/video/fbdev/simplefb.c
> > +++ b/drivers/video/fbdev/simplefb.c
> > @@ -84,6 +84,10 @@ struct simplefb_par {
> >   static void simplefb_clocks_destroy(struct simplefb_par *par);
> >   static void simplefb_regulators_destroy(struct simplefb_par *par);
> > +/*
> > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> > + * of unregister_framebuffer() or fb_release(). Do any cleanup here.
> > + */
> >   static void simplefb_destroy(struct fb_info *info)
> >   {
> >   	struct simplefb_par *par = info->par;
> > @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
> >   	if (info->screen_base)
> >   		iounmap(info->screen_base);
> > +	framebuffer_release(info);
> > +
> >   	if (mem)
> >   		release_mem_region(mem->start, resource_size(mem));
> 
> The original problem with fbdev hot-unplug was that vmwgfx needed the
> framebuffer region to be released. If we release it only after userspace
> closed it's final file descriptor, vmwgfx could have already failed.
> 
> I still don't fully get why this code apparently works or at least doesn't
> blow up occasionally. Any ideas?

See my other reply, releasing hw stuff should be done from ->remove, not
->fb_destroy.

Also note that none of the patches discussed moved around anything here,
we simply leaked things a bit when only unregistering the fb and not going
through the device->remove callback. I guess in practice it works because
unregistering the device sends out a uevent, and userspace then closes all
it's device fd as a reaction to that, and usually that's fast enough to
not upset anyone?

No idea tbh.
-Daniel

> Best regards
> Thomas
> 
> >   }
> > @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev)
> >   {
> >   	struct fb_info *info = platform_get_drvdata(pdev);
> > +	/* simplefb_destroy takes care of info cleanup */
> >   	unregister_framebuffer(info);
> > -	framebuffer_release(info);
> >   	return 0;
> >   }
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch