* [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use ?
@ 2011-05-23 20:11 Laurent Pinchart
2011-05-23 20:53 ` [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use Florian Tobias Schandinat
2011-05-23 22:07 ` Florian Tobias Schandinat
0 siblings, 2 replies; 6+ messages in thread
From: Laurent Pinchart @ 2011-05-23 20:11 UTC (permalink / raw)
To: linux-fbdev
Hi everybody,
I've recently received a patch for my fbdev test application to fix a panning-
related issue. The application naively assumed that only the xoffset and
yoffset were used, and the patch commit message explained that several drivers
relied on other fields being filled with current values.
I got curious about this. As there's no FBIOPAN_DISPLAY documentation that
clearly states which fields are used and which fields must be ignored by
drivers, I checked the in-tree fbdev drivers and here's what I found.
First of all, the FBIOPAN_DISPLAY implementation (drivers/video/fbmem.c) uses
the xoffset, yoffset and vmode fields. That was expected, there's nothing too
weird there. Several drivers also use the activate field, which seems like a
valid use to me.
Then, two drivers (video/msm and staging/msm) use the reserved fields. As
usage of the reserved fields is by definition not standard, I decided to
ignore this.
Finally, here's a list of the drivers using other fields, along with the
fields that are being used.
media/video/ivtv - xres_virtual, bits_per_pixel
video/68328fb - xres, yres
video/acornfb - yres, yres_virtual
video/arkfb - bits_per_pixel, xres_virtual
video/atmel_lcdfb - bits_per_pixel, xres_virtual, xres
video/aty/radeon - xres, yres, xres_virtual, yres_virtual, bits_per_pixel
video/da8xx-fb - bits_per_pixel
video/fb-puv3 - xres, yres
video/g364fb - xres, yres, yres_virtual
video/gxt4500 - xres, yres, xres_virtual, yres_virtual
video/hgafb - xres, yres
video/imsttfb - bits_per_pixel
video/intelfb - xres, yres, xres_virtual, yres_virtual, bits_per_pixel
video/mb862xx - xres_virtual, yres_virtual
video/mx3fb - yres, xres_virtual, bits_per_pixel
video/neofb - xres_virtual, bits_per_pixel
video/pm2fb - bits_per_pixel, xres
video/pm3fb - xres, bits_per_pixel
video/s3c-fb - yres
video/s3fb - bits_per_pixel, xres_virtual
video/savage - xres_virtual, bits_per_pixel
video/sh_mobile_lcdcfb - nonstd, xres, yres_virtual
video/sis - xres, yres, xres_virtual, yres_virtual, bits_per_pixel
video/sm501fb - xres_virtual, yres_virtual, bits_per_pixel
video/tridentfb - xres_virtual, bits_per_pixel
video/vfb - xres, yres
video/vga16fb - bits_per_pixel
video/via - xres_virtual, bits_per_pixel
video/vt8500lcdfb - xres, xres_virtual
video/vt8623fb - bits_per_pixel, xres_virtual
video/xgifb - xres_virtual, yres_virtual, xres, yres, bits_per_pixel
These fields are used to validate the xoffset and yoffset parameters against
the active resolution, as well as to compute offset in vram.
This clearly looks like bugs to me, especially given that many other drivers
compute the same parameters using info->var instead of var. For instance, if
drivers/video/aty/radeon_base.c implements the pan display operation as
if ((var->xoffset + var->xres > var->xres_virtual)
|| (var->yoffset + var->yres > var->yres_virtual))
return -EINVAL;
...
OUTREG(CRTC_OFFSET, ((var->yoffset * var->xres_virtual + var->xoffset)
* var->bits_per_pixel / 8) & ~7);
If var isn't a copy of the current fb parameters, the code will clearly lead
to a wrong behaviour. Even worse, some drivers use var to validate parameters
and then info->var to compute offsets, or the other way around. This could
lead to security issues.
I believe all those drivers should be fixed to use info->var instead of var to
access the current xres, yres, xres_virtual, yres_virtual and bits_per_pixel
fields in their pan display operation handler. However, this could break
applications that rely on the current buggy behaviour. Would that be
acceptable ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use
2011-05-23 20:11 [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use ? Laurent Pinchart
@ 2011-05-23 20:53 ` Florian Tobias Schandinat
2011-05-23 22:07 ` Florian Tobias Schandinat
1 sibling, 0 replies; 6+ messages in thread
From: Florian Tobias Schandinat @ 2011-05-23 20:53 UTC (permalink / raw)
To: linux-fbdev
Hi Laurent,
On 05/23/2011 08:11 PM, Laurent Pinchart wrote:
> Hi everybody,
>
> I've recently received a patch for my fbdev test application to fix a panning-
> related issue. The application naively assumed that only the xoffset and
> yoffset were used, and the patch commit message explained that several drivers
> relied on other fields being filled with current values.
>
> I got curious about this. As there's no FBIOPAN_DISPLAY documentation that
> clearly states which fields are used and which fields must be ignored by
> drivers, I checked the in-tree fbdev drivers and here's what I found.
>
> First of all, the FBIOPAN_DISPLAY implementation (drivers/video/fbmem.c) uses
> the xoffset, yoffset and vmode fields. That was expected, there's nothing too
> weird there. Several drivers also use the activate field, which seems like a
> valid use to me.
>
> Then, two drivers (video/msm and staging/msm) use the reserved fields. As
> usage of the reserved fields is by definition not standard, I decided to
> ignore this.
>
> Finally, here's a list of the drivers using other fields, along with the
> fields that are being used.
>
> media/video/ivtv - xres_virtual, bits_per_pixel
> video/68328fb - xres, yres
> video/acornfb - yres, yres_virtual
> video/arkfb - bits_per_pixel, xres_virtual
> video/atmel_lcdfb - bits_per_pixel, xres_virtual, xres
> video/aty/radeon - xres, yres, xres_virtual, yres_virtual, bits_per_pixel
> video/da8xx-fb - bits_per_pixel
> video/fb-puv3 - xres, yres
> video/g364fb - xres, yres, yres_virtual
> video/gxt4500 - xres, yres, xres_virtual, yres_virtual
> video/hgafb - xres, yres
> video/imsttfb - bits_per_pixel
> video/intelfb - xres, yres, xres_virtual, yres_virtual, bits_per_pixel
> video/mb862xx - xres_virtual, yres_virtual
> video/mx3fb - yres, xres_virtual, bits_per_pixel
> video/neofb - xres_virtual, bits_per_pixel
> video/pm2fb - bits_per_pixel, xres
> video/pm3fb - xres, bits_per_pixel
> video/s3c-fb - yres
> video/s3fb - bits_per_pixel, xres_virtual
> video/savage - xres_virtual, bits_per_pixel
> video/sh_mobile_lcdcfb - nonstd, xres, yres_virtual
> video/sis - xres, yres, xres_virtual, yres_virtual, bits_per_pixel
> video/sm501fb - xres_virtual, yres_virtual, bits_per_pixel
> video/tridentfb - xres_virtual, bits_per_pixel
> video/vfb - xres, yres
> video/vga16fb - bits_per_pixel
> video/via - xres_virtual, bits_per_pixel
> video/vt8500lcdfb - xres, xres_virtual
> video/vt8623fb - bits_per_pixel, xres_virtual
> video/xgifb - xres_virtual, yres_virtual, xres, yres, bits_per_pixel
>
> These fields are used to validate the xoffset and yoffset parameters against
> the active resolution, as well as to compute offset in vram.
>
> This clearly looks like bugs to me, especially given that many other drivers
> compute the same parameters using info->var instead of var. For instance, if
> drivers/video/aty/radeon_base.c implements the pan display operation as
>
> if ((var->xoffset + var->xres> var->xres_virtual)
> || (var->yoffset + var->yres> var->yres_virtual))
> return -EINVAL;
>
> ...
>
> OUTREG(CRTC_OFFSET, ((var->yoffset * var->xres_virtual + var->xoffset)
> * var->bits_per_pixel / 8)& ~7);
>
> If var isn't a copy of the current fb parameters, the code will clearly lead
> to a wrong behaviour. Even worse, some drivers use var to validate parameters
> and then info->var to compute offsets, or the other way around. This could
> lead to security issues.
You're right.
> I believe all those drivers should be fixed to use info->var instead of var to
> access the current xres, yres, xres_virtual, yres_virtual and bits_per_pixel
> fields in their pan display operation handler. However, this could break
> applications that rely on the current buggy behaviour. Would that be
> acceptable ?
Yes, as it is obviously wrong to assume other fields of var are valid (otherwise
there would be no need for pan at all). So let's fix those drivers. I'll fix
video/via ASAP (there it's also a bug to use xres_virtual as fix.line_length is
more correct (alignment)) and have a look at the others as time permits (though
it would be better if people fix them who have the ability to test).
Thanks a lot,
Florian Tobias Schandinat
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] viafb: use display information in info not in var for panning
2011-05-23 20:11 [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use ? Laurent Pinchart
@ 2011-05-23 22:07 ` Florian Tobias Schandinat
2011-05-23 22:07 ` Florian Tobias Schandinat
1 sibling, 0 replies; 6+ messages in thread
From: Florian Tobias Schandinat @ 2011-05-23 21:51 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, Florian Tobias Schandinat, stable
As Laurent pointed out we must not use any information in the passed
var besides xoffset, yoffset and vmode as otherwise applications
might abuse it. Also use the aligned fix.line_length and not the
(possible) unaligned xres_virtual.
Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: stable@kernel.org
---
drivers/video/via/viafbdev.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
index 3114a87..aa87529 100644
--- a/drivers/video/via/viafbdev.c
+++ b/drivers/video/via/viafbdev.c
@@ -348,8 +348,9 @@ static int viafb_pan_display(struct fb_var_screeninfo *var,
struct fb_info *info)
{
struct viafb_par *viapar = info->par;
- u32 vram_addr = (var->yoffset * var->xres_virtual + var->xoffset)
- * (var->bits_per_pixel / 8) + viapar->vram_addr;
+ u32 vram_addr = viapar->vram_addr
+ + var->yoffset * info->fix.line_length
+ + var->xoffset * info->var.bits_per_pixel / 8;
DEBUG_MSG(KERN_DEBUG "viafb_pan_display, address = %d\n", vram_addr);
if (!viafb_dual_fb) {
--
1.6.3.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] viafb: use display information in info not in var for panning
@ 2011-05-23 22:07 ` Florian Tobias Schandinat
0 siblings, 0 replies; 6+ messages in thread
From: Florian Tobias Schandinat @ 2011-05-23 22:07 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, Florian Tobias Schandinat, stable
As Laurent pointed out we must not use any information in the passed
var besides xoffset, yoffset and vmode as otherwise applications
might abuse it. Also use the aligned fix.line_length and not the
(possible) unaligned xres_virtual.
Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: stable@kernel.org
---
drivers/video/via/viafbdev.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
index 3114a87..aa87529 100644
--- a/drivers/video/via/viafbdev.c
+++ b/drivers/video/via/viafbdev.c
@@ -348,8 +348,9 @@ static int viafb_pan_display(struct fb_var_screeninfo *var,
struct fb_info *info)
{
struct viafb_par *viapar = info->par;
- u32 vram_addr = (var->yoffset * var->xres_virtual + var->xoffset)
- * (var->bits_per_pixel / 8) + viapar->vram_addr;
+ u32 vram_addr = viapar->vram_addr
+ + var->yoffset * info->fix.line_length
+ + var->xoffset * info->var.bits_per_pixel / 8;
DEBUG_MSG(KERN_DEBUG "viafb_pan_display, address = %d\n", vram_addr);
if (!viafb_dual_fb) {
--
1.6.3.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] viafb: use display information in info not in var for panning
2011-05-23 22:07 ` Florian Tobias Schandinat
@ 2011-05-25 9:41 ` Laurent Pinchart
-1 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2011-05-25 9:41 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, linux-kernel, stable
Hi Florian,
On Tuesday 24 May 2011 00:07:26 Florian Tobias Schandinat wrote:
> As Laurent pointed out we must not use any information in the passed
> var besides xoffset, yoffset and vmode as otherwise applications
> might abuse it. Also use the aligned fix.line_length and not the
> (possible) unaligned xres_virtual.
>
> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: stable@kernel.org
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/via/viafbdev.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
> index 3114a87..aa87529 100644
> --- a/drivers/video/via/viafbdev.c
> +++ b/drivers/video/via/viafbdev.c
> @@ -348,8 +348,9 @@ static int viafb_pan_display(struct fb_var_screeninfo
> *var, struct fb_info *info)
> {
> struct viafb_par *viapar = info->par;
> - u32 vram_addr = (var->yoffset * var->xres_virtual + var->xoffset)
> - * (var->bits_per_pixel / 8) + viapar->vram_addr;
> + u32 vram_addr = viapar->vram_addr
> + + var->yoffset * info->fix.line_length
> + + var->xoffset * info->var.bits_per_pixel / 8;
>
> DEBUG_MSG(KERN_DEBUG "viafb_pan_display, address = %d\n", vram_addr);
> if (!viafb_dual_fb) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] viafb: use display information in info not in var for panning
@ 2011-05-25 9:41 ` Laurent Pinchart
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2011-05-25 9:41 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, linux-kernel, stable
Hi Florian,
On Tuesday 24 May 2011 00:07:26 Florian Tobias Schandinat wrote:
> As Laurent pointed out we must not use any information in the passed
> var besides xoffset, yoffset and vmode as otherwise applications
> might abuse it. Also use the aligned fix.line_length and not the
> (possible) unaligned xres_virtual.
>
> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: stable@kernel.org
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/via/viafbdev.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
> index 3114a87..aa87529 100644
> --- a/drivers/video/via/viafbdev.c
> +++ b/drivers/video/via/viafbdev.c
> @@ -348,8 +348,9 @@ static int viafb_pan_display(struct fb_var_screeninfo
> *var, struct fb_info *info)
> {
> struct viafb_par *viapar = info->par;
> - u32 vram_addr = (var->yoffset * var->xres_virtual + var->xoffset)
> - * (var->bits_per_pixel / 8) + viapar->vram_addr;
> + u32 vram_addr = viapar->vram_addr
> + + var->yoffset * info->fix.line_length
> + + var->xoffset * info->var.bits_per_pixel / 8;
>
> DEBUG_MSG(KERN_DEBUG "viafb_pan_display, address = %d\n", vram_addr);
> if (!viafb_dual_fb) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-25 9:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-23 20:11 [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use ? Laurent Pinchart
2011-05-23 20:53 ` [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use Florian Tobias Schandinat
2011-05-23 21:51 ` [PATCH] viafb: use display information in info not in var for panning Florian Tobias Schandinat
2011-05-23 22:07 ` Florian Tobias Schandinat
2011-05-25 9:41 ` Laurent Pinchart
2011-05-25 9:41 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.