| Message ID | 20230428122452.4856-18-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm,fbdev: Use fbdev's I/O helpers | expand |
On Fri, Apr 28, 2023 at 2:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Push the test for info->screen_base from fb_read() and fb_write() into > the implementations of struct fb_ops.{fb_read,fb_write}. In cases where > the driver operates on info->screen_buffer, test this field instead. > > While bothi fields, screen_base and screen_buffer, are stored in the both > same location, they refer to different address spaces. For correctness, > we want to test each field in exactly the code that uses it. Not a direct comment for this patch: and later the union can be split in two separate fields, to protect against misuse? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Am 03.05.23 um 11:51 schrieb Geert Uytterhoeven: > On Fri, Apr 28, 2023 at 2:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Push the test for info->screen_base from fb_read() and fb_write() into >> the implementations of struct fb_ops.{fb_read,fb_write}. In cases where >> the driver operates on info->screen_buffer, test this field instead. >> >> While bothi fields, screen_base and screen_buffer, are stored in the > > both > >> same location, they refer to different address spaces. For correctness, >> we want to test each field in exactly the code that uses it. > > Not a direct comment for this patch: and later the union can be split > in two separate fields, to protect against misuse? No idea. Currently we have sparse that warns about mismatching address spaces if the fields are mixed up. That's good enough, as far I'm concerned. Best regards Thomas > > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Thomas, On Wed, May 3, 2023 at 4:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Am 03.05.23 um 11:51 schrieb Geert Uytterhoeven: > > On Fri, Apr 28, 2023 at 2:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> Push the test for info->screen_base from fb_read() and fb_write() into > >> the implementations of struct fb_ops.{fb_read,fb_write}. In cases where > >> the driver operates on info->screen_buffer, test this field instead. > >> > >> While bothi fields, screen_base and screen_buffer, are stored in the > > > > both > > > >> same location, they refer to different address spaces. For correctness, > >> we want to test each field in exactly the code that uses it. > > > > Not a direct comment for this patch: and later the union can be split > > in two separate fields, to protect against misuse? > > No idea. Currently we have sparse that warns about mismatching address > spaces if the fields are mixed up. That's good enough, as far I'm concerned. The potential issue that is still present is that an fbdev driver uses fb_info.screen_base, and configures the use of drawing ops that use fb_info.screen_buffer (or vice-versa), which will happily use the wrong type of pointer. Sparse doesn't protect against that. Gr{oetje,eeting}s, Geert
Hi Am 03.05.23 um 17:02 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Wed, May 3, 2023 at 4:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Am 03.05.23 um 11:51 schrieb Geert Uytterhoeven: >>> On Fri, Apr 28, 2023 at 2:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> Push the test for info->screen_base from fb_read() and fb_write() into >>>> the implementations of struct fb_ops.{fb_read,fb_write}. In cases where >>>> the driver operates on info->screen_buffer, test this field instead. >>>> >>>> While bothi fields, screen_base and screen_buffer, are stored in the >>> >>> both >>> >>>> same location, they refer to different address spaces. For correctness, >>>> we want to test each field in exactly the code that uses it. >>> >>> Not a direct comment for this patch: and later the union can be split >>> in two separate fields, to protect against misuse? >> >> No idea. Currently we have sparse that warns about mismatching address >> spaces if the fields are mixed up. That's good enough, as far I'm concerned. > > The potential issue that is still present is that an fbdev driver uses > fb_info.screen_base, and configures the use of drawing ops that use > fb_info.screen_buffer (or vice-versa), which will happily use the wrong > type of pointer. Sparse doesn't protect against that. Right. From a quick grep, I've found quite a cases where cfb_ functions operate on non-__iomem memory. I'm sure that the opposite with sys_ functions exists as well. Fixing this will be a good follow-up patchset. Thanks for the suggestion. Best regards Thomas > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c index 22123a25daea..0aeb9daaee4c 100644 --- a/drivers/media/pci/ivtv/ivtvfb.c +++ b/drivers/media/pci/ivtv/ivtvfb.c @@ -378,6 +378,9 @@ static ssize_t ivtvfb_write(struct fb_info *info, const char __user *buf, unsigned long dma_size; u16 lead = 0, tail = 0; + if (!info->screen_base) + return -ENODEV; + total_size = info->screen_size; if (total_size == 0) diff --git a/drivers/video/fbdev/arcfb.c b/drivers/video/fbdev/arcfb.c index 088c4b30fd31..7750e020839e 100644 --- a/drivers/video/fbdev/arcfb.c +++ b/drivers/video/fbdev/arcfb.c @@ -451,6 +451,9 @@ static ssize_t arcfb_write(struct fb_info *info, const char __user *buf, struct arcfb_par *par; unsigned int xres; + if (!info->screen_buffer) + return -ENODEV; + p = *ppos; par = info->par; xres = info->var.xres; diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c index 691de5df581b..e9c5d5c04062 100644 --- a/drivers/video/fbdev/broadsheetfb.c +++ b/drivers/video/fbdev/broadsheetfb.c @@ -1013,6 +1013,9 @@ static ssize_t broadsheetfb_write(struct fb_info *info, const char __user *buf, int err = 0; unsigned long total_size; + if (!info->screen_buffer) + return -ENODEV; + total_size = info->fix.smem_len; if (p > total_size) diff --git a/drivers/video/fbdev/cobalt_lcdfb.c b/drivers/video/fbdev/cobalt_lcdfb.c index 5f8b6324d2e8..26dbd1c78195 100644 --- a/drivers/video/fbdev/cobalt_lcdfb.c +++ b/drivers/video/fbdev/cobalt_lcdfb.c @@ -129,6 +129,9 @@ static ssize_t cobalt_lcdfb_read(struct fb_info *info, char __user *buf, unsigned long pos; int len, retval = 0; + if (!info->screen_base) + return -ENODEV; + pos = *ppos; if (pos >= LCD_CHARS_MAX || count == 0) return 0; @@ -175,6 +178,9 @@ static ssize_t cobalt_lcdfb_write(struct fb_info *info, const char __user *buf, unsigned long pos; int len, retval = 0; + if (!info->screen_base) + return -ENODEV; + pos = *ppos; if (pos >= LCD_CHARS_MAX || count == 0) return 0; diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c index 7dee5d3c7fb1..0cb0989abda6 100644 --- a/drivers/video/fbdev/core/fb_sys_fops.c +++ b/drivers/video/fbdev/core/fb_sys_fops.c @@ -22,6 +22,9 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count, unsigned long total_size, c; ssize_t ret; + if (!info->screen_buffer) + return -ENODEV; + total_size = info->screen_size; if (total_size == 0) @@ -61,6 +64,9 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf, unsigned long total_size, c; size_t ret; + if (!info->screen_buffer) + return -ENODEV; + total_size = info->screen_size; if (total_size == 0) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index b0881348c27f..3a80d13afd26 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -768,7 +768,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) int c, cnt = 0, err = 0; unsigned long total_size, trailing; - if (!info || ! info->screen_base) + if (!info) return -ENODEV; if (info->state != FBINFO_STATE_RUNNING) @@ -777,6 +777,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) if (info->fbops->fb_read) return info->fbops->fb_read(info, buf, count, ppos); + if (!info->screen_base) + return -ENODEV; + total_size = info->screen_size; if (total_size == 0) @@ -836,7 +839,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) int c, cnt = 0, err = 0; unsigned long total_size, trailing; - if (!info || !info->screen_base) + if (!info) return -ENODEV; if (info->state != FBINFO_STATE_RUNNING) @@ -845,6 +848,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) if (info->fbops->fb_write) return info->fbops->fb_write(info, buf, count, ppos); + if (!info->screen_base) + return -ENODEV; + total_size = info->screen_size; if (total_size == 0) diff --git a/drivers/video/fbdev/hecubafb.c b/drivers/video/fbdev/hecubafb.c index a2996d39f918..72308d4e0c22 100644 --- a/drivers/video/fbdev/hecubafb.c +++ b/drivers/video/fbdev/hecubafb.c @@ -163,6 +163,9 @@ static ssize_t hecubafb_write(struct fb_info *info, const char __user *buf, int err = 0; unsigned long total_size; + if (!info->screen_buffer) + return -ENODEV; + total_size = info->fix.smem_len; if (p > total_size) diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c index 2bb068cadac6..7fc59466fe6c 100644 --- a/drivers/video/fbdev/metronomefb.c +++ b/drivers/video/fbdev/metronomefb.c @@ -523,6 +523,9 @@ static ssize_t metronomefb_write(struct fb_info *info, const char __user *buf, int err = 0; unsigned long total_size; + if (!info->screen_buffer) + return -ENODEV; + total_size = info->fix.smem_len; if (p > total_size) diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c index 6888127a5eb8..550fdb5b4d41 100644 --- a/drivers/video/fbdev/pvr2fb.c +++ b/drivers/video/fbdev/pvr2fb.c @@ -647,6 +647,9 @@ static ssize_t pvr2fb_write(struct fb_info *info, const char *buf, struct page **pages; int ret, i; + if (!info->screen_base) + return -ENODEV; + nr_pages = (count + PAGE_SIZE - 1) >> PAGE_SHIFT; pages = kmalloc_array(nr_pages, sizeof(struct page *), GFP_KERNEL); diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c index 6f852cd756c5..b7ad3c644e13 100644 --- a/drivers/video/fbdev/sm712fb.c +++ b/drivers/video/fbdev/sm712fb.c @@ -1028,7 +1028,7 @@ static ssize_t smtcfb_read(struct fb_info *info, char __user *buf, int c, i, cnt = 0, err = 0; unsigned long total_size; - if (!info || !info->screen_base) + if (!info->screen_base) return -ENODEV; total_size = info->screen_size; @@ -1091,7 +1091,7 @@ static ssize_t smtcfb_write(struct fb_info *info, const char __user *buf, int c, i, cnt = 0, err = 0; unsigned long total_size; - if (!info || !info->screen_base) + if (!info->screen_base) return -ENODEV; total_size = info->screen_size; diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c index 046b9990d27c..a8f2975de76b 100644 --- a/drivers/video/fbdev/ssd1307fb.c +++ b/drivers/video/fbdev/ssd1307fb.c @@ -301,6 +301,9 @@ static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf, void *dst; int ret; + if (!info->screen_buffer) + return -ENODEV; + total_size = info->fix.smem_len; if (p > total_size)
Push the test for info->screen_base from fb_read() and fb_write() into the implementations of struct fb_ops.{fb_read,fb_write}. In cases where the driver operates on info->screen_buffer, test this field instead. While bothi fields, screen_base and screen_buffer, are stored in the same location, they refer to different address spaces. For correctness, we want to test each field in exactly the code that uses it. v2: * also test screen_base in pvr2fb (Geert) * also test screen_buffer in ivtvfb, arcfb, broadsheetfb, hecubafb, metronomefb and ssd1307fb (Geert) * give a rational for the change (Geert) Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Acked-by: Helge Deller <deller@gmx.de> --- drivers/media/pci/ivtv/ivtvfb.c | 3 +++ drivers/video/fbdev/arcfb.c | 3 +++ drivers/video/fbdev/broadsheetfb.c | 3 +++ drivers/video/fbdev/cobalt_lcdfb.c | 6 ++++++ drivers/video/fbdev/core/fb_sys_fops.c | 6 ++++++ drivers/video/fbdev/core/fbmem.c | 10 ++++++++-- drivers/video/fbdev/hecubafb.c | 3 +++ drivers/video/fbdev/metronomefb.c | 3 +++ drivers/video/fbdev/pvr2fb.c | 3 +++ drivers/video/fbdev/sm712fb.c | 4 ++-- drivers/video/fbdev/ssd1307fb.c | 3 +++ 11 files changed, 43 insertions(+), 4 deletions(-)