[RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use ?

[RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use ?

[Laurent Pinchart]

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

Re: [RFC] Which fb_var_screeninfo fields should FBIOPAN_DISPLAY use

[Florian Tobias Schandinat]

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

[PATCH] viafb: use display information in info not in var for panning

[Florian Tobias Schandinat]

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

Re: [PATCH] viafb: use display information in info not in var for panning

[Laurent Pinchart]

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