A reference to the framebuffer device struct fb_info is stored in the file
private data, but this reference could no longer be valid and must not be
accessed directly. Instead, the file_fb_info() accessor function must be
used since it does sanity checking to make sure that the fb_info is valid.
This can happen for example if the registered framebuffer device is for a
driver that just uses a framebuffer provided by the system firmware. In
that case, the fbdev core would unregister the framebuffer device when a
real video driver is probed and ask to remove conflicting framebuffers.
Most fbdev file operations already use the helper to get the fb_info but
get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
exported. Rename it and add a fb_ prefix to denote that is public now.
Reported-by: Junxiao Chang <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
Changes in v2:
- Fix copy & paste error passing file->private_data instead of file
to fb_file_fb_info() function (Sam Ravnborg).
drivers/video/fbdev/core/fb_defio.c | 5 ++++-
drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++---------
include/linux/fb.h | 1 +
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 842c66b3e33d..ccdf903c48bd 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync)
{
- struct fb_info *info = file->private_data;
+ struct fb_info *info = fb_file_fb_info(file);
struct inode *inode = file_inode(file);
int err = file_write_and_wait_range(file, start, end);
if (err)
return err;
+ if (!info)
+ return -ENODEV;
+
/* Skip if deferred io is compiled-in but disabled on this fbdev */
if (!info->fbdefio)
return 0;
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 97eb0dee411c..ba2c14a1087d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = {
* So look up the fb_info using the inode minor number,
* and just verify it against the reference we have.
*/
-static struct fb_info *file_fb_info(struct file *file)
+struct fb_info *fb_file_fb_info(struct file *file)
{
struct inode *inode = file_inode(file);
int fbidx = iminor(inode);
@@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file)
info = NULL;
return info;
}
+EXPORT_SYMBOL(fb_file_fb_info);
static ssize_t
fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
u8 *buffer, *dst;
u8 __iomem *src;
int c, cnt = 0, err = 0;
@@ -825,7 +826,7 @@ static ssize_t
fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
u8 *buffer, *src;
u8 __iomem *dst;
int c, cnt = 0, err = 0;
@@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
if (!info)
return -ENODEV;
@@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd,
static long fb_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
const struct fb_ops *fb;
long ret = -ENOIOCTLCMD;
@@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
static int
fb_mmap(struct file *file, struct vm_area_struct * vma)
{
- struct fb_info *info = file_fb_info(file);
+ struct fb_info *info = fb_file_fb_info(file);
int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
unsigned long mmio_pgoff;
unsigned long start;
@@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
- struct fb_info * const info = file_fb_info(file);
+ struct fb_info * const info = fb_file_fb_info(file);
if (!info)
return -ENODEV;
@@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long addr, unsigned long len,
unsigned long pgoff, unsigned long flags)
{
- struct fb_info * const info = filp->private_data;
- unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);
+ struct fb_info * const info = fb_file_fb_info(filp);
+ unsigned long fb_size;
+
+ if (!info)
+ return -ENODEV;
+
+ fb_size = PAGE_ALIGN(info->fix.smem_len);
if (pgoff > fb_size || len > fb_size - pgoff)
return -EINVAL;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 9a77ab615c36..3004b8b8c5c2 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
struct fb_fix_screeninfo *fix);
extern int fb_get_options(const char *name, char **option);
extern int fb_new_modelist(struct fb_info *info);
+extern struct fb_info *fb_file_fb_info(struct file *file);
extern struct fb_info *registered_fb[FB_MAX];
extern int num_registered_fb;
--
2.35.1
On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
> A reference to the framebuffer device struct fb_info is stored in the file
> private data, but this reference could no longer be valid and must not be
> accessed directly. Instead, the file_fb_info() accessor function must be
> used since it does sanity checking to make sure that the fb_info is valid.
>
> This can happen for example if the registered framebuffer device is for a
> driver that just uses a framebuffer provided by the system firmware. In
> that case, the fbdev core would unregister the framebuffer device when a
> real video driver is probed and ask to remove conflicting framebuffers.
>
> Most fbdev file operations already use the helper to get the fb_info but
> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>
> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
> exported. Rename it and add a fb_ prefix to denote that is public now.
>
> Reported-by: Junxiao Chang <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Acked-by: Sam Ravnborg <[email protected]>
Hi
Am 03.05.22 um 22:19 schrieb Javier Martinez Canillas:
> A reference to the framebuffer device struct fb_info is stored in the file
> private data, but this reference could no longer be valid and must not be
> accessed directly. Instead, the file_fb_info() accessor function must be
> used since it does sanity checking to make sure that the fb_info is valid.
>
> This can happen for example if the registered framebuffer device is for a
> driver that just uses a framebuffer provided by the system firmware. In
> that case, the fbdev core would unregister the framebuffer device when a
> real video driver is probed and ask to remove conflicting framebuffers.
>
> Most fbdev file operations already use the helper to get the fb_info but
> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>
> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
> exported. Rename it and add a fb_ prefix to denote that is public now.
>
> Reported-by: Junxiao Chang <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
Please see my comment below.
> ---
>
> Changes in v2:
> - Fix copy & paste error passing file->private_data instead of file
> to fb_file_fb_info() function (Sam Ravnborg).
>
> drivers/video/fbdev/core/fb_defio.c | 5 ++++-
> drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++---------
> include/linux/fb.h | 1 +
> 3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 842c66b3e33d..ccdf903c48bd 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>
> int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> {
> - struct fb_info *info = file->private_data;
> + struct fb_info *info = fb_file_fb_info(file);
> struct inode *inode = file_inode(file);
> int err = file_write_and_wait_range(file, start, end);
> if (err)
> return err;
>
> + if (!info)
> + return -ENODEV;
> +
This is consistent with other functions, but it's probably not the
correct errno code. It means that a device is not available for opening.
But the situation here is rather as with close() on a
disconnected-network file. The call to close() returns EIO in this case.
Maybe we should consider changing this in a separate patch.
Best regards
Thomas
> /* Skip if deferred io is compiled-in but disabled on this fbdev */
> if (!info->fbdefio)
> return 0;
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 97eb0dee411c..ba2c14a1087d 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = {
> * So look up the fb_info using the inode minor number,
> * and just verify it against the reference we have.
> */
> -static struct fb_info *file_fb_info(struct file *file)
> +struct fb_info *fb_file_fb_info(struct file *file)
> {
> struct inode *inode = file_inode(file);
> int fbidx = iminor(inode);
> @@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file)
> info = NULL;
> return info;
> }
> +EXPORT_SYMBOL(fb_file_fb_info);
>
> static ssize_t
> fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> u8 *buffer, *dst;
> u8 __iomem *src;
> int c, cnt = 0, err = 0;
> @@ -825,7 +826,7 @@ static ssize_t
> fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> u8 *buffer, *src;
> u8 __iomem *dst;
> int c, cnt = 0, err = 0;
> @@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>
> static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
>
> if (!info)
> return -ENODEV;
> @@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd,
> static long fb_compat_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> const struct fb_ops *fb;
> long ret = -ENOIOCTLCMD;
>
> @@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
> static int
> fb_mmap(struct file *file, struct vm_area_struct * vma)
> {
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
> unsigned long mmio_pgoff;
> unsigned long start;
> @@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file)
> __acquires(&info->lock)
> __releases(&info->lock)
> {
> - struct fb_info * const info = file_fb_info(file);
> + struct fb_info * const info = fb_file_fb_info(file);
>
> if (!info)
> return -ENODEV;
> @@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long addr, unsigned long len,
> unsigned long pgoff, unsigned long flags)
> {
> - struct fb_info * const info = filp->private_data;
> - unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);
> + struct fb_info * const info = fb_file_fb_info(filp);
> + unsigned long fb_size;
> +
> + if (!info)
> + return -ENODEV;
> +
> + fb_size = PAGE_ALIGN(info->fix.smem_len);
>
> if (pgoff > fb_size || len > fb_size - pgoff)
> return -EINVAL;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 9a77ab615c36..3004b8b8c5c2 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
> struct fb_fix_screeninfo *fix);
> extern int fb_get_options(const char *name, char **option);
> extern int fb_new_modelist(struct fb_info *info);
> +extern struct fb_info *fb_file_fb_info(struct file *file);
>
> extern struct fb_info *registered_fb[FB_MAX];
> extern int num_registered_fb;
--
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
Hi
Am 04.05.22 um 11:02 schrieb Daniel Vetter:
> On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
>> A reference to the framebuffer device struct fb_info is stored in the file
>> private data, but this reference could no longer be valid and must not be
>> accessed directly. Instead, the file_fb_info() accessor function must be
>> used since it does sanity checking to make sure that the fb_info is valid.
>>
>> This can happen for example if the registered framebuffer device is for a
>> driver that just uses a framebuffer provided by the system firmware. In
>> that case, the fbdev core would unregister the framebuffer device when a
>> real video driver is probed and ask to remove conflicting framebuffers.
>>
>> Most fbdev file operations already use the helper to get the fb_info but
>> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>>
>> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
>> exported. Rename it and add a fb_ prefix to denote that is public now.
>>
>> Reported-by: Junxiao Chang <[email protected]>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> Note that fb_file_info is hilariously racy since there's nothing
> preventing a concurrenct framebuffer_unregister. Or at least I'm not
> seeing anything. See cf4a3ae4ef33 ("fbdev: lock_fb_info cannot fail") for
> context, maybe reference that commit here in your patch.
Can we acquire registration_lock around [1]?
[1]
https://elixir.bootlin.com/linux/v5.17.5/source/drivers/video/fbdev/core/fbmem.c#L756
>
> Either way this doesn't really make anything worse, so
> Acked-by: Daniel Vetter <[email protected]>
>
> Cheers, Daniel
>
>> ---
>>
>> Changes in v2:
>> - Fix copy & paste error passing file->private_data instead of file
>> to fb_file_fb_info() function (Sam Ravnborg).
>>
>> drivers/video/fbdev/core/fb_defio.c | 5 ++++-
>> drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++---------
>> include/linux/fb.h | 1 +
>> 3 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index 842c66b3e33d..ccdf903c48bd 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>>
>> int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>> {
>> - struct fb_info *info = file->private_data;
>> + struct fb_info *info = fb_file_fb_info(file);
>> struct inode *inode = file_inode(file);
>> int err = file_write_and_wait_range(file, start, end);
>> if (err)
>> return err;
>>
>> + if (!info)
>> + return -ENODEV;
>> +
>> /* Skip if deferred io is compiled-in but disabled on this fbdev */
>> if (!info->fbdefio)
>> return 0;
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 97eb0dee411c..ba2c14a1087d 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = {
>> * So look up the fb_info using the inode minor number,
>> * and just verify it against the reference we have.
>> */
>> -static struct fb_info *file_fb_info(struct file *file)
>> +struct fb_info *fb_file_fb_info(struct file *file)
>> {
>> struct inode *inode = file_inode(file);
>> int fbidx = iminor(inode);
>> @@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file)
>> info = NULL;
>> return info;
>> }
>> +EXPORT_SYMBOL(fb_file_fb_info);
>>
>> static ssize_t
>> fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> {
>> unsigned long p = *ppos;
>> - struct fb_info *info = file_fb_info(file);
>> + struct fb_info *info = fb_file_fb_info(file);
>> u8 *buffer, *dst;
>> u8 __iomem *src;
>> int c, cnt = 0, err = 0;
>> @@ -825,7 +826,7 @@ static ssize_t
>> fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>> {
>> unsigned long p = *ppos;
>> - struct fb_info *info = file_fb_info(file);
>> + struct fb_info *info = fb_file_fb_info(file);
>> u8 *buffer, *src;
>> u8 __iomem *dst;
>> int c, cnt = 0, err = 0;
>> @@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>
>> static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> {
>> - struct fb_info *info = file_fb_info(file);
>> + struct fb_info *info = fb_file_fb_info(file);
>>
>> if (!info)
>> return -ENODEV;
>> @@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd,
>> static long fb_compat_ioctl(struct file *file, unsigned int cmd,
>> unsigned long arg)
>> {
>> - struct fb_info *info = file_fb_info(file);
>> + struct fb_info *info = fb_file_fb_info(file);
>> const struct fb_ops *fb;
>> long ret = -ENOIOCTLCMD;
>>
>> @@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
>> static int
>> fb_mmap(struct file *file, struct vm_area_struct * vma)
>> {
>> - struct fb_info *info = file_fb_info(file);
>> + struct fb_info *info = fb_file_fb_info(file);
>> int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
>> unsigned long mmio_pgoff;
>> unsigned long start;
>> @@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file)
>> __acquires(&info->lock)
>> __releases(&info->lock)
>> {
>> - struct fb_info * const info = file_fb_info(file);
>> + struct fb_info * const info = fb_file_fb_info(file);
>>
>> if (!info)
>> return -ENODEV;
>> @@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp,
>> unsigned long addr, unsigned long len,
>> unsigned long pgoff, unsigned long flags)
>> {
>> - struct fb_info * const info = filp->private_data;
>> - unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);
>> + struct fb_info * const info = fb_file_fb_info(filp);
>> + unsigned long fb_size;
>> +
>> + if (!info)
>> + return -ENODEV;
>> +
>> + fb_size = PAGE_ALIGN(info->fix.smem_len);
>>
>> if (pgoff > fb_size || len > fb_size - pgoff)
>> return -EINVAL;
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index 9a77ab615c36..3004b8b8c5c2 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
>> struct fb_fix_screeninfo *fix);
>> extern int fb_get_options(const char *name, char **option);
>> extern int fb_new_modelist(struct fb_info *info);
>> +extern struct fb_info *fb_file_fb_info(struct file *file);
>>
>> extern struct fb_info *registered_fb[FB_MAX];
>> extern int num_registered_fb;
>> --
>> 2.35.1
>>
>
--
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
Hello Thomas,
On 5/4/22 10:15, Thomas Zimmermann wrote:
> Hi
>
> Am 03.05.22 um 22:19 schrieb Javier Martinez Canillas:
>> A reference to the framebuffer device struct fb_info is stored in the file
>> private data, but this reference could no longer be valid and must not be
>> accessed directly. Instead, the file_fb_info() accessor function must be
>> used since it does sanity checking to make sure that the fb_info is valid.
>>
>> This can happen for example if the registered framebuffer device is for a
>> driver that just uses a framebuffer provided by the system firmware. In
>> that case, the fbdev core would unregister the framebuffer device when a
>> real video driver is probed and ask to remove conflicting framebuffers.
>>
>> Most fbdev file operations already use the helper to get the fb_info but
>> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>>
>> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
>> exported. Rename it and add a fb_ prefix to denote that is public now.
>>
>> Reported-by: Junxiao Chang <[email protected]>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> Reviewed-by: Thomas Zimmermann <[email protected]>
>
Thanks.
> Please see my comment below.
[snip]
>>
>> + if (!info)
>> + return -ENODEV;
>> +
>
> This is consistent with other functions, but it's probably not the
> correct errno code. It means that a device is not available for opening.
>
> But the situation here is rather as with close() on a
> disconnected-network file. The call to close() returns EIO in this case.
> Maybe we should consider changing this in a separate patch.
>
Indeed. Agree that -EIO makes more sense here.
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Hello Daniel,
On 5/4/22 11:02, Daniel Vetter wrote:
> On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
>> A reference to the framebuffer device struct fb_info is stored in the file
>> private data, but this reference could no longer be valid and must not be
>> accessed directly. Instead, the file_fb_info() accessor function must be
>> used since it does sanity checking to make sure that the fb_info is valid.
>>
>> This can happen for example if the registered framebuffer device is for a
>> driver that just uses a framebuffer provided by the system firmware. In
>> that case, the fbdev core would unregister the framebuffer device when a
>> real video driver is probed and ask to remove conflicting framebuffers.
>>
>> Most fbdev file operations already use the helper to get the fb_info but
>> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>>
>> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
>> exported. Rename it and add a fb_ prefix to denote that is public now.
>>
>> Reported-by: Junxiao Chang <[email protected]>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> Note that fb_file_info is hilariously racy since there's nothing
> preventing a concurrenct framebuffer_unregister. Or at least I'm not
> seeing anything. See cf4a3ae4ef33 ("fbdev: lock_fb_info cannot fail") for
> context, maybe reference that commit here in your patch.
>
> Either way this doesn't really make anything worse, so
> Acked-by: Daniel Vetter <[email protected]>
>
Yes, I noticed is racy but at least checking this makes less likely to
occur. And thanks, I'll reference that commit in the description of v3.
BTW, I also noticed that the same race that happens with open(),read(),
close(), etc happens with the VM operations:
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
...
vma->vm_private_data = info;
...
}
static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
{
...
struct fb_info *info = vmf->vma->vm_private_data;
...
}
static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
{
...
struct fb_info *info = vmf->vma->vm_private_data;
...
}
So something similar to fb_file_fb_info() is needed to check if
the vm_private_data is still valid. I guess that could be done
by using the vmf->vma->vm_file and attempting the same trick that
fb_file_fb_info() does ?
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
On Wed, May 04, 2022 at 11:28:07AM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
>
> On 5/4/22 11:02, Daniel Vetter wrote:
> > On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
> >> A reference to the framebuffer device struct fb_info is stored in the file
> >> private data, but this reference could no longer be valid and must not be
> >> accessed directly. Instead, the file_fb_info() accessor function must be
> >> used since it does sanity checking to make sure that the fb_info is valid.
> >>
> >> This can happen for example if the registered framebuffer device is for a
> >> driver that just uses a framebuffer provided by the system firmware. In
> >> that case, the fbdev core would unregister the framebuffer device when a
> >> real video driver is probed and ask to remove conflicting framebuffers.
> >>
> >> Most fbdev file operations already use the helper to get the fb_info but
> >> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
> >>
> >> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
> >> exported. Rename it and add a fb_ prefix to denote that is public now.
> >>
> >> Reported-by: Junxiao Chang <[email protected]>
> >> Signed-off-by: Javier Martinez Canillas <[email protected]>
> >
> > Note that fb_file_info is hilariously racy since there's nothing
> > preventing a concurrenct framebuffer_unregister. Or at least I'm not
> > seeing anything. See cf4a3ae4ef33 ("fbdev: lock_fb_info cannot fail") for
> > context, maybe reference that commit here in your patch.
> >
> > Either way this doesn't really make anything worse, so
> > Acked-by: Daniel Vetter <[email protected]>
> >
>
> Yes, I noticed is racy but at least checking this makes less likely to
> occur. And thanks, I'll reference that commit in the description of v3.
>
> BTW, I also noticed that the same race that happens with open(),read(),
> close(), etc happens with the VM operations:
>
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> ...
> vma->vm_private_data = info;
> ...
> }
>
> static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> {
> ...
> struct fb_info *info = vmf->vma->vm_private_data;
> ...
> }
>
> static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> {
> ...
> struct fb_info *info = vmf->vma->vm_private_data;
> ...
> }
>
> So something similar to fb_file_fb_info() is needed to check if
> the vm_private_data is still valid. I guess that could be done
> by using the vmf->vma->vm_file and attempting the same trick that
> fb_file_fb_info() does ?
Yeah should work, except if the ptes are set up already there's kinda not
much that this will prevent. We'd need to tear down mappings and SIGBUS or
alternatively have something else in place there so userspace doesn't blow
up in funny ways (which is what we're doing on the drm side, or at least
trying to).
I'm also not sure how much we should care, since ideally for drm drivers
this is all taken care of by drm_dev_enter in the right places. It does
mean though that fbdev mmap either needs to have it's own memory or be
fully redirected to the drm gem mmap.
And then we can afford to just not care to fix fbdev itself.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hello Thomas,
On 5/4/22 13:08, Thomas Zimmermann wrote:
[snip]
>>> So something similar to fb_file_fb_info() is needed to check if
>>> the vm_private_data is still valid. I guess that could be done
>>> by using the vmf->vma->vm_file and attempting the same trick that
>>> fb_file_fb_info() does ?
>>
>> Yeah should work, except if the ptes are set up already there's kinda not
>> much that this will prevent. We'd need to tear down mappings and SIGBUS or
>> alternatively have something else in place there so userspace doesn't blow
>> up in funny ways (which is what we're doing on the drm side, or at least
>> trying to).
>>
>> I'm also not sure how much we should care, since ideally for drm drivers
>> this is all taken care of by drm_dev_enter in the right places. It does
>> mean though that fbdev mmap either needs to have it's own memory or be
>> fully redirected to the drm gem mmap.
>>
>> And then we can afford to just not care to fix fbdev itself.
>
> While the problem has been there ever since, the bug didn't happen until
> we fixed hot-unplugging for fbdev. Not doing anything is probably not
> the right thing.
>
Actually, this issue shouldn't happen if the fbdev drivers are not buggy
and do the proper cleanup at .fb_release() time rather than at .remove().
I'll post patches for simplefb and efifb which are the drivers that we
mostly care at this point. So we should be good and not need more fixes.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
> A reference to the framebuffer device struct fb_info is stored in the file
> private data, but this reference could no longer be valid and must not be
> accessed directly. Instead, the file_fb_info() accessor function must be
> used since it does sanity checking to make sure that the fb_info is valid.
>
> This can happen for example if the registered framebuffer device is for a
> driver that just uses a framebuffer provided by the system firmware. In
> that case, the fbdev core would unregister the framebuffer device when a
> real video driver is probed and ask to remove conflicting framebuffers.
>
> Most fbdev file operations already use the helper to get the fb_info but
> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>
> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
> exported. Rename it and add a fb_ prefix to denote that is public now.
>
> Reported-by: Junxiao Chang <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Note that fb_file_info is hilariously racy since there's nothing
preventing a concurrenct framebuffer_unregister. Or at least I'm not
seeing anything. See cf4a3ae4ef33 ("fbdev: lock_fb_info cannot fail") for
context, maybe reference that commit here in your patch.
Either way this doesn't really make anything worse, so
Acked-by: Daniel Vetter <[email protected]>
Cheers, Daniel
> ---
>
> Changes in v2:
> - Fix copy & paste error passing file->private_data instead of file
> to fb_file_fb_info() function (Sam Ravnborg).
>
> drivers/video/fbdev/core/fb_defio.c | 5 ++++-
> drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++---------
> include/linux/fb.h | 1 +
> 3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index 842c66b3e33d..ccdf903c48bd 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>
> int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> {
> - struct fb_info *info = file->private_data;
> + struct fb_info *info = fb_file_fb_info(file);
> struct inode *inode = file_inode(file);
> int err = file_write_and_wait_range(file, start, end);
> if (err)
> return err;
>
> + if (!info)
> + return -ENODEV;
> +
> /* Skip if deferred io is compiled-in but disabled on this fbdev */
> if (!info->fbdefio)
> return 0;
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 97eb0dee411c..ba2c14a1087d 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = {
> * So look up the fb_info using the inode minor number,
> * and just verify it against the reference we have.
> */
> -static struct fb_info *file_fb_info(struct file *file)
> +struct fb_info *fb_file_fb_info(struct file *file)
> {
> struct inode *inode = file_inode(file);
> int fbidx = iminor(inode);
> @@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file)
> info = NULL;
> return info;
> }
> +EXPORT_SYMBOL(fb_file_fb_info);
>
> static ssize_t
> fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> u8 *buffer, *dst;
> u8 __iomem *src;
> int c, cnt = 0, err = 0;
> @@ -825,7 +826,7 @@ static ssize_t
> fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> u8 *buffer, *src;
> u8 __iomem *dst;
> int c, cnt = 0, err = 0;
> @@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>
> static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
>
> if (!info)
> return -ENODEV;
> @@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd,
> static long fb_compat_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> const struct fb_ops *fb;
> long ret = -ENOIOCTLCMD;
>
> @@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
> static int
> fb_mmap(struct file *file, struct vm_area_struct * vma)
> {
> - struct fb_info *info = file_fb_info(file);
> + struct fb_info *info = fb_file_fb_info(file);
> int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
> unsigned long mmio_pgoff;
> unsigned long start;
> @@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file)
> __acquires(&info->lock)
> __releases(&info->lock)
> {
> - struct fb_info * const info = file_fb_info(file);
> + struct fb_info * const info = fb_file_fb_info(file);
>
> if (!info)
> return -ENODEV;
> @@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long addr, unsigned long len,
> unsigned long pgoff, unsigned long flags)
> {
> - struct fb_info * const info = filp->private_data;
> - unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len);
> + struct fb_info * const info = fb_file_fb_info(filp);
> + unsigned long fb_size;
> +
> + if (!info)
> + return -ENODEV;
> +
> + fb_size = PAGE_ALIGN(info->fix.smem_len);
>
> if (pgoff > fb_size || len > fb_size - pgoff)
> return -EINVAL;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 9a77ab615c36..3004b8b8c5c2 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
> struct fb_fix_screeninfo *fix);
> extern int fb_get_options(const char *name, char **option);
> extern int fb_new_modelist(struct fb_info *info);
> +extern struct fb_info *fb_file_fb_info(struct file *file);
>
> extern struct fb_info *registered_fb[FB_MAX];
> extern int num_registered_fb;
> --
> 2.35.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi
Am 04.05.22 um 12:55 schrieb Daniel Vetter:
> On Wed, May 04, 2022 at 11:28:07AM +0200, Javier Martinez Canillas wrote:
>> Hello Daniel,
>>
>> On 5/4/22 11:02, Daniel Vetter wrote:
>>> On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
>>>> A reference to the framebuffer device struct fb_info is stored in the file
>>>> private data, but this reference could no longer be valid and must not be
>>>> accessed directly. Instead, the file_fb_info() accessor function must be
>>>> used since it does sanity checking to make sure that the fb_info is valid.
>>>>
>>>> This can happen for example if the registered framebuffer device is for a
>>>> driver that just uses a framebuffer provided by the system firmware. In
>>>> that case, the fbdev core would unregister the framebuffer device when a
>>>> real video driver is probed and ask to remove conflicting framebuffers.
>>>>
>>>> Most fbdev file operations already use the helper to get the fb_info but
>>>> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>>>>
>>>> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
>>>> exported. Rename it and add a fb_ prefix to denote that is public now.
>>>>
>>>> Reported-by: Junxiao Chang <[email protected]>
>>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>>>
>>> Note that fb_file_info is hilariously racy since there's nothing
>>> preventing a concurrenct framebuffer_unregister. Or at least I'm not
>>> seeing anything. See cf4a3ae4ef33 ("fbdev: lock_fb_info cannot fail") for
>>> context, maybe reference that commit here in your patch.
>>>
>>> Either way this doesn't really make anything worse, so
>>> Acked-by: Daniel Vetter <[email protected]>
>>>
>>
>> Yes, I noticed is racy but at least checking this makes less likely to
>> occur. And thanks, I'll reference that commit in the description of v3.
>>
>> BTW, I also noticed that the same race that happens with open(),read(),
>> close(), etc happens with the VM operations:
>>
>> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>> {
>> ...
>> vma->vm_private_data = info;
>> ...
>> }
>>
>> static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
>> {
>> ...
>> struct fb_info *info = vmf->vma->vm_private_data;
>> ...
>> }
>>
>> static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>> {
>> ...
>> struct fb_info *info = vmf->vma->vm_private_data;
>> ...
>> }
>>
>> So something similar to fb_file_fb_info() is needed to check if
>> the vm_private_data is still valid. I guess that could be done
>> by using the vmf->vma->vm_file and attempting the same trick that
>> fb_file_fb_info() does ?
>
> Yeah should work, except if the ptes are set up already there's kinda not
> much that this will prevent. We'd need to tear down mappings and SIGBUS or
> alternatively have something else in place there so userspace doesn't blow
> up in funny ways (which is what we're doing on the drm side, or at least
> trying to).
>
> I'm also not sure how much we should care, since ideally for drm drivers
> this is all taken care of by drm_dev_enter in the right places. It does
> mean though that fbdev mmap either needs to have it's own memory or be
> fully redirected to the drm gem mmap.
>
> And then we can afford to just not care to fix fbdev itself.
While the problem has been there ever since, the bug didn't happen until
we fixed hot-unplugging for fbdev. Not doing anything is probably not
the right thing.
Best regards
Thomas
> -Daniel
--
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