* [PATCH AUTOSEL 6.12 022/486] fbdev: fsl-diu-fb: add missing device_remove_file()
From: Sasha Levin @ 2025-05-05 22:31 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Shixiong Ou, Helge Deller, Sasha Levin, timur, linux-fbdev,
dri-devel
In-Reply-To: <20250505223922.2682012-1-sashal@kernel.org>
From: Shixiong Ou <oushixiong@kylinos.cn>
[ Upstream commit 86d16cd12efa547ed43d16ba7a782c1251c80ea8 ]
Call device_remove_file() when driver remove.
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/video/fbdev/fsl-diu-fb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/fsl-diu-fb.c b/drivers/video/fbdev/fsl-diu-fb.c
index 5ac8201c35337..b71d15794ce8b 100644
--- a/drivers/video/fbdev/fsl-diu-fb.c
+++ b/drivers/video/fbdev/fsl-diu-fb.c
@@ -1827,6 +1827,7 @@ static void fsl_diu_remove(struct platform_device *pdev)
int i;
data = dev_get_drvdata(&pdev->dev);
+ device_remove_file(&pdev->dev, &data->dev_attr);
disable_lcdc(&data->fsl_diu_info[0]);
free_irq(data->irq, data->diu_reg);
--
2.39.5
^ permalink raw reply related
* [PATCH AUTOSEL 6.14 026/642] fbdev: core: tileblit: Implement missing margin clearing for tileblit
From: Sasha Levin @ 2025-05-05 22:04 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Zsolt Kajtar, Helge Deller, Sasha Levin, simona, linux-fbdev,
dri-devel
In-Reply-To: <20250505221419.2672473-1-sashal@kernel.org>
From: Zsolt Kajtar <soci@c64.rulez.org>
[ Upstream commit 76d3ca89981354e1f85a3e0ad9ac4217d351cc72 ]
I was wondering why there's garbage at the bottom of the screen when
tile blitting is used with an odd mode like 1080, 600 or 200. Sure there's
only space for half a tile but the same area is clean when the buffer
is bitmap.
Then later I found that it's supposed to be cleaned but that's not
implemented. So I took what's in bitblit and adapted it for tileblit.
This implementation was tested for both the horizontal and vertical case,
and now does the same as what's done for bitmap buffers.
If anyone is interested to reproduce the problem then I could bet that'd
be on a S3 or Ark. Just set up a mode with an odd line count and make
sure that the virtual size covers the complete tile at the bottom. E.g.
for 600 lines that's 608 virtual lines for a 16 tall tile. Then the
bottom area should be cleaned.
For the right side it's more difficult as there the drivers won't let an
odd size happen, unless the code is modified. But once it reports back a
few pixel columns short then fbcon won't use the last column. With the
patch that column is now clean.
Btw. the virtual size should be rounded up by the driver for both axes
(not only the horizontal) so that it's dividable by the tile size.
That's a driver bug but correcting it is not in scope for this patch.
Implement missing margin clearing for tileblit
Signed-off-by: Zsolt Kajtar <soci@c64.rulez.org>
Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/video/fbdev/core/tileblit.c | 37 ++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/tileblit.c b/drivers/video/fbdev/core/tileblit.c
index 45b0828fad1cf..d342b90c42b7f 100644
--- a/drivers/video/fbdev/core/tileblit.c
+++ b/drivers/video/fbdev/core/tileblit.c
@@ -74,7 +74,42 @@ static void tile_putcs(struct vc_data *vc, struct fb_info *info,
static void tile_clear_margins(struct vc_data *vc, struct fb_info *info,
int color, int bottom_only)
{
- return;
+ unsigned int cw = vc->vc_font.width;
+ unsigned int ch = vc->vc_font.height;
+ unsigned int rw = info->var.xres - (vc->vc_cols*cw);
+ unsigned int bh = info->var.yres - (vc->vc_rows*ch);
+ unsigned int rs = info->var.xres - rw;
+ unsigned int bs = info->var.yres - bh;
+ unsigned int vwt = info->var.xres_virtual / cw;
+ unsigned int vht = info->var.yres_virtual / ch;
+ struct fb_tilerect rect;
+
+ rect.index = vc->vc_video_erase_char &
+ ((vc->vc_hi_font_mask) ? 0x1ff : 0xff);
+ rect.fg = color;
+ rect.bg = color;
+
+ if ((int) rw > 0 && !bottom_only) {
+ rect.sx = (info->var.xoffset + rs + cw - 1) / cw;
+ rect.sy = 0;
+ rect.width = (rw + cw - 1) / cw;
+ rect.height = vht;
+ if (rect.width + rect.sx > vwt)
+ rect.width = vwt - rect.sx;
+ if (rect.sx < vwt)
+ info->tileops->fb_tilefill(info, &rect);
+ }
+
+ if ((int) bh > 0) {
+ rect.sx = info->var.xoffset / cw;
+ rect.sy = (info->var.yoffset + bs) / ch;
+ rect.width = rs / cw;
+ rect.height = (bh + ch - 1) / ch;
+ if (rect.height + rect.sy > vht)
+ rect.height = vht - rect.sy;
+ if (rect.sy < vht)
+ info->tileops->fb_tilefill(info, &rect);
+ }
}
static void tile_cursor(struct vc_data *vc, struct fb_info *info, bool enable,
--
2.39.5
^ permalink raw reply related
* [PATCH AUTOSEL 6.14 025/642] fbcon: Use correct erase colour for clearing in fbcon
From: Sasha Levin @ 2025-05-05 22:04 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Zsolt Kajtar, Helge Deller, Sasha Levin, simona, qianqiang.liu,
jfalempe, oushixiong, linux-fbdev, dri-devel
In-Reply-To: <20250505221419.2672473-1-sashal@kernel.org>
From: Zsolt Kajtar <soci@c64.rulez.org>
[ Upstream commit 892c788d73fe4a94337ed092cb998c49fa8ecaf4 ]
The erase colour calculation for fbcon clearing should use get_color instead
of attr_col_ec, like everything else. The latter is similar but is not correct.
For example it's missing the depth dependent remapping and doesn't care about
blanking.
The problem can be reproduced by setting up the background colour to grey
(vt.color=0x70) and having an fbcon console set to 2bpp (4 shades of gray).
Now the background attribute should be 1 (dark gray) on the console.
If the screen is scrolled when pressing enter in a shell prompt at the bottom
line then the new line is cleared using colour 7 instead of 1. That's not
something fillrect likes (at 2bbp it expect 0-3) so the result is interesting.
This patch switches to get_color with vc_video_erase_char to determine the
erase colour from attr_col_ec. That makes the latter function redundant as
no other users were left.
Use correct erase colour for clearing in fbcon
Signed-off-by: Zsolt Kajtar <soci@c64.rulez.org>
Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/video/fbdev/core/bitblit.c | 5 ++--
drivers/video/fbdev/core/fbcon.c | 10 +++++---
drivers/video/fbdev/core/fbcon.h | 38 +---------------------------
drivers/video/fbdev/core/fbcon_ccw.c | 5 ++--
drivers/video/fbdev/core/fbcon_cw.c | 5 ++--
drivers/video/fbdev/core/fbcon_ud.c | 5 ++--
drivers/video/fbdev/core/tileblit.c | 8 +++---
7 files changed, 18 insertions(+), 58 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index 3ff1b2a8659e8..f9475c14f7339 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -59,12 +59,11 @@ static void bit_bmove(struct vc_data *vc, struct fb_info *info, int sy,
}
static void bit_clear(struct vc_data *vc, struct fb_info *info, int sy,
- int sx, int height, int width)
+ int sx, int height, int width, int fg, int bg)
{
- int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
struct fb_fillrect region;
- region.color = attr_bgcol_ec(bgshift, vc, info);
+ region.color = bg;
region.dx = sx * vc->vc_font.width;
region.dy = sy * vc->vc_font.height;
region.width = width * vc->vc_font.width;
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index e8b4e8c119b5c..07d127110ca4c 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -1258,7 +1258,7 @@ static void __fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
{
struct fb_info *info = fbcon_info_from_console(vc->vc_num);
struct fbcon_ops *ops = info->fbcon_par;
-
+ int fg, bg;
struct fbcon_display *p = &fb_display[vc->vc_num];
u_int y_break;
@@ -1279,16 +1279,18 @@ static void __fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
fbcon_clear_margins(vc, 0);
}
+ fg = get_color(vc, info, vc->vc_video_erase_char, 1);
+ bg = get_color(vc, info, vc->vc_video_erase_char, 0);
/* Split blits that cross physical y_wrap boundary */
y_break = p->vrows - p->yscroll;
if (sy < y_break && sy + height - 1 >= y_break) {
u_int b = y_break - sy;
- ops->clear(vc, info, real_y(p, sy), sx, b, width);
+ ops->clear(vc, info, real_y(p, sy), sx, b, width, fg, bg);
ops->clear(vc, info, real_y(p, sy + b), sx, height - b,
- width);
+ width, fg, bg);
} else
- ops->clear(vc, info, real_y(p, sy), sx, height, width);
+ ops->clear(vc, info, real_y(p, sy), sx, height, width, fg, bg);
}
static void fbcon_clear(struct vc_data *vc, unsigned int sy, unsigned int sx,
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index df70ea5ec5b37..4d97e6d8a16a2 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -55,7 +55,7 @@ struct fbcon_ops {
void (*bmove)(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int dy, int dx, int height, int width);
void (*clear)(struct vc_data *vc, struct fb_info *info, int sy,
- int sx, int height, int width);
+ int sx, int height, int width, int fb, int bg);
void (*putcs)(struct vc_data *vc, struct fb_info *info,
const unsigned short *s, int count, int yy, int xx,
int fg, int bg);
@@ -116,42 +116,6 @@ static inline int mono_col(const struct fb_info *info)
return (~(0xfff << max_len)) & 0xff;
}
-static inline int attr_col_ec(int shift, struct vc_data *vc,
- struct fb_info *info, int is_fg)
-{
- int is_mono01;
- int col;
- int fg;
- int bg;
-
- if (!vc)
- return 0;
-
- if (vc->vc_can_do_color)
- return is_fg ? attr_fgcol(shift,vc->vc_video_erase_char)
- : attr_bgcol(shift,vc->vc_video_erase_char);
-
- if (!info)
- return 0;
-
- col = mono_col(info);
- is_mono01 = info->fix.visual == FB_VISUAL_MONO01;
-
- if (attr_reverse(vc->vc_video_erase_char)) {
- fg = is_mono01 ? col : 0;
- bg = is_mono01 ? 0 : col;
- }
- else {
- fg = is_mono01 ? 0 : col;
- bg = is_mono01 ? col : 0;
- }
-
- return is_fg ? fg : bg;
-}
-
-#define attr_bgcol_ec(bgshift, vc, info) attr_col_ec(bgshift, vc, info, 0)
-#define attr_fgcol_ec(fgshift, vc, info) attr_col_ec(fgshift, vc, info, 1)
-
/*
* Scroll Method
*/
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index f9b794ff7d396..89ef4ba7e8672 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -78,14 +78,13 @@ static void ccw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
}
static void ccw_clear(struct vc_data *vc, struct fb_info *info, int sy,
- int sx, int height, int width)
+ int sx, int height, int width, int fg, int bg)
{
struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
- int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
u32 vyres = GETVYRES(ops->p, info);
- region.color = attr_bgcol_ec(bgshift,vc,info);
+ region.color = bg;
region.dx = sy * vc->vc_font.height;
region.dy = vyres - ((sx + width) * vc->vc_font.width);
region.height = width * vc->vc_font.width;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index 903f6fc174e14..b9dac7940fb77 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -63,14 +63,13 @@ static void cw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
}
static void cw_clear(struct vc_data *vc, struct fb_info *info, int sy,
- int sx, int height, int width)
+ int sx, int height, int width, int fg, int bg)
{
struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
- int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
u32 vxres = GETVXRES(ops->p, info);
- region.color = attr_bgcol_ec(bgshift,vc,info);
+ region.color = bg;
region.dx = vxres - ((sy + height) * vc->vc_font.height);
region.dy = sx * vc->vc_font.width;
region.height = width * vc->vc_font.width;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 594331936fd3c..0af7913a2abdc 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -64,15 +64,14 @@ static void ud_bmove(struct vc_data *vc, struct fb_info *info, int sy,
}
static void ud_clear(struct vc_data *vc, struct fb_info *info, int sy,
- int sx, int height, int width)
+ int sx, int height, int width, int fg, int bg)
{
struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
- int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
u32 vyres = GETVYRES(ops->p, info);
u32 vxres = GETVXRES(ops->p, info);
- region.color = attr_bgcol_ec(bgshift,vc,info);
+ region.color = bg;
region.dy = vyres - ((sy + height) * vc->vc_font.height);
region.dx = vxres - ((sx + width) * vc->vc_font.width);
region.width = width * vc->vc_font.width;
diff --git a/drivers/video/fbdev/core/tileblit.c b/drivers/video/fbdev/core/tileblit.c
index eff7ec4da1671..45b0828fad1cf 100644
--- a/drivers/video/fbdev/core/tileblit.c
+++ b/drivers/video/fbdev/core/tileblit.c
@@ -32,16 +32,14 @@ static void tile_bmove(struct vc_data *vc, struct fb_info *info, int sy,
}
static void tile_clear(struct vc_data *vc, struct fb_info *info, int sy,
- int sx, int height, int width)
+ int sx, int height, int width, int fg, int bg)
{
struct fb_tilerect rect;
- int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
- int fgshift = (vc->vc_hi_font_mask) ? 9 : 8;
rect.index = vc->vc_video_erase_char &
((vc->vc_hi_font_mask) ? 0x1ff : 0xff);
- rect.fg = attr_fgcol_ec(fgshift, vc, info);
- rect.bg = attr_bgcol_ec(bgshift, vc, info);
+ rect.fg = fg;
+ rect.bg = bg;
rect.sx = sx;
rect.sy = sy;
rect.width = width;
--
2.39.5
^ permalink raw reply related
* [PATCH AUTOSEL 6.14 024/642] fbdev: fsl-diu-fb: add missing device_remove_file()
From: Sasha Levin @ 2025-05-05 22:04 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Shixiong Ou, Helge Deller, Sasha Levin, timur, linux-fbdev,
dri-devel
In-Reply-To: <20250505221419.2672473-1-sashal@kernel.org>
From: Shixiong Ou <oushixiong@kylinos.cn>
[ Upstream commit 86d16cd12efa547ed43d16ba7a782c1251c80ea8 ]
Call device_remove_file() when driver remove.
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/video/fbdev/fsl-diu-fb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/fsl-diu-fb.c b/drivers/video/fbdev/fsl-diu-fb.c
index 5ac8201c35337..b71d15794ce8b 100644
--- a/drivers/video/fbdev/fsl-diu-fb.c
+++ b/drivers/video/fbdev/fsl-diu-fb.c
@@ -1827,6 +1827,7 @@ static void fsl_diu_remove(struct platform_device *pdev)
int i;
data = dev_get_drvdata(&pdev->dev);
+ device_remove_file(&pdev->dev, &data->dev_attr);
disable_lcdc(&data->fsl_diu_info[0]);
free_irq(data->irq, data->diu_reg);
--
2.39.5
^ permalink raw reply related
* Re: [PATCH] video: fbdev: arkfb: Cast ics5342_init() allocation type
From: Geert Uytterhoeven @ 2025-05-02 7:36 UTC (permalink / raw)
To: Helge Deller
Cc: Kees Cook, Javier Martinez Canillas, Thomas Zimmermann, Zheyu Ma,
Samuel Thibault, Jiapeng Chong, linux-fbdev, dri-devel,
linux-kernel, linux-hardening
In-Reply-To: <e68c6218-6055-45a6-b96e-9c8381a4b409@gmx.de>
Hi Helge,
On Tue, 29 Apr 2025 at 22:17, Helge Deller <deller@gmx.de> wrote:
> On 4/28/25 08:36, Geert Uytterhoeven wrote:
> > On Sat, 26 Apr 2025 at 13:33, Helge Deller <deller@gmx.de> wrote:
> >> On 4/26/25 08:23, Kees Cook wrote:
> >>> In preparation for making the kmalloc family of allocators type aware,
> >>> we need to make sure that the returned type from the allocation matches
> >>> the type of the variable being assigned. (Before, the allocator would
> >>> always return "void *", which can be implicitly cast to any pointer type.)
> >>>
> >>> The assigned type is "struct dac_info *" but the returned type will be
> >>> "struct ics5342_info *", which has a larger allocation size. This is
> >>> by design, as struct ics5342_info contains struct dac_info as its first
> >>> member. Cast the allocation type to match the assignment.
> >>>
> >>> Signed-off-by: Kees Cook <kees@kernel.org>
> >
> > Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
> > arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.
> >
> >> I applied your patch, but wouldn't this untested patch be cleaner and fulfill the
> >> same purpose to match a kzalloc return type?
> >>
> >> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> >> index 7d131e3d159a..a57c8a992e11 100644
> >> --- a/drivers/video/fbdev/arkfb.c
> >> +++ b/drivers/video/fbdev/arkfb.c
> >> @@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
> >>
> >> static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
> >> {
> >> - struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
> >> + struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
> >
> > sizeof(*ics_info)?
> >
> >> + struct dac_info *info = &ics_info->dac;
> >
> > Exactly my thought when I noticed this commit. Adding casts makes
> > it harder to notice any future discrepancies.
>
> I've changed it accordingly.
Thanks, but the one-line summary no longer matches what the commit
is doing...
Commit f1a78a7d7827357c ("fbdev: arkfb: Cast ics5342_init() allocation
type") in fbdev/for-next.
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
^ permalink raw reply
* [PATCH v2 3/3] fbdev: hyperv_fb: Fix mmap of framebuffers allocated using alloc_pages()
From: mhkelley58 @ 2025-05-02 4:05 UTC (permalink / raw)
To: simona, deller, haiyangz, kys, wei.liu, decui, akpm
Cc: weh, tzimmermann, hch, dri-devel, linux-fbdev, linux-kernel,
linux-hyperv, linux-mm
In-Reply-To: <20250502040525.822075-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@outlook.com>
Framebuffer memory allocated using alloc_pages() was added to hyperv_fb in
commit 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb
on HyperV Gen 1 VMs.") in kernel version 5.6. But mmap'ing such
framebuffers into user space has never worked due to limitations in the
kind of memory that fbdev deferred I/O works with. As a result of the
limitation, hyperv_fb's usage results in memory free lists becoming corrupt
and Linux eventually panics.
With support for framebuffers allocated using alloc_pages() recently added
to fbdev deferred I/O, fix the problem by setting the flag telling fbdev
deferred I/O to use the new support.
Fixes: 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.")
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
drivers/video/fbdev/hyperv_fb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 75338ffc703f..1698221f857e 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -1020,6 +1020,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
info->fix.smem_len = screen_fb_size;
info->screen_base = par->mmio_vp;
info->screen_size = screen_fb_size;
+ info->flags |= FBINFO_KMEMFB;
par->need_docopy = false;
goto getmem_done;
--
2.25.1
^ permalink raw reply related
* [PATCH v2 2/3] fbdev/deferred-io: Support contiguous kernel memory framebuffers
From: mhkelley58 @ 2025-05-02 4:05 UTC (permalink / raw)
To: simona, deller, haiyangz, kys, wei.liu, decui, akpm
Cc: weh, tzimmermann, hch, dri-devel, linux-fbdev, linux-kernel,
linux-hyperv, linux-mm
In-Reply-To: <20250502040525.822075-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@outlook.com>
Current defio code works only for framebuffer memory that is allocated
with vmalloc(). The code assumes that the underlying page refcount can
be used by the mm subsystem to manage each framebuffer page's lifecycle,
including freeing the page if the refcount goes to 0. This approach is
consistent with vmalloc'ed memory, but not with contiguous kernel memory
allocated via alloc_pages() or similar. The latter such memory pages
usually have a refcount of 0 when allocated, and would be incorrectly
freed page-by-page if used with defio. That free'ing corrupts the memory
free lists and Linux eventually panics. Simply bumping the refcount after
allocation doesn’t work because when the framebuffer memory is freed,
__free_pages() complains about non-zero refcounts.
Commit 37b4837959cb ("video: deferred io with physically contiguous
memory") from the year 2008 purported to add support for contiguous
kernel memory framebuffers. The motivating device, sh_mobile_lcdcfb, uses
dma_alloc_coherent() to allocate framebuffer memory, which is likely to
use alloc_pages(). It's unclear to me how this commit actually worked at
the time, unless dma_alloc_coherent() was pulling from a CMA pool instead
of alloc_pages(). Or perhaps alloc_pages() worked differently or on the
arm32 architecture on which sh_mobile_lcdcfb is used.
In any case, for x86 and arm64 today, commit 37b4837959cb9 is not
sufficient to support contiguous kernel memory framebuffers. The problem
can be seen with the hyperv_fb driver, which may allocate the framebuffer
memory using vmalloc() or alloc_pages(), depending on the configuration
of the Hyper-V guest VM (Gen 1 vs. Gen 2) and the size of the framebuffer.
Fix this limitation by adding defio support for contiguous kernel memory
framebuffers. A driver with a framebuffer allocated from contiguous
kernel memory must set the FBINFO_KMEMFB flag to indicate such.
Tested with the hyperv_fb driver in both configurations -- with a vmalloc()
framebuffer and with an alloc_pages() framebuffer on x86. Also verified a
vmalloc() framebuffer on arm64. Hardware is not available to me to verify
that the older arm32 devices still work correctly, but the path for
vmalloc() framebuffers is essentially unchanged.
Even with these changes, defio does not support framebuffers in MMIO
space, as defio code depends on framebuffer memory pages having
corresponding 'struct page's.
Fixes: 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.")
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
Changes in v2:
* Tweaked code comments regarding framebuffers allocated with
dma_alloc_coherent() [Christoph Hellwig]
drivers/video/fbdev/core/fb_defio.c | 128 +++++++++++++++++++++++-----
include/linux/fb.h | 1 +
2 files changed, 109 insertions(+), 20 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 4fc93f253e06..f8ae91a1c4df 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -8,11 +8,40 @@
* for more details.
*/
+/*
+ * Deferred I/O ("defio") allows framebuffers that are mmap()'ed to user space
+ * to batch user space writes into periodic updates to the underlying
+ * framebuffer hardware or other implementation (such as with a virtualized
+ * framebuffer in a VM). At each batch interval, a callback is invoked in the
+ * framebuffer's kernel driver, and the callback is supplied with a list of
+ * pages that have been modified in the preceding interval. The callback can
+ * use this information to update the framebuffer hardware as necessary. The
+ * batching can improve performance and reduce the overhead of updating the
+ * hardware.
+ *
+ * Defio is supported on framebuffers allocated using vmalloc() and allocated
+ * as contiguous kernel memory using alloc_pages() or kmalloc(). These
+ * memory allocations all have corresponding "struct page"s. Framebuffers
+ * allocated using dma_alloc_coherent() should not be used with defio.
+ * Such allocations should be treated as a black box owned by the DMA
+ * layer, and should not be deconstructed into individual pages as defio
+ * does. Framebuffers in MMIO space are *not* supported because MMIO space
+ * does not have corrresponding "struct page"s.
+ *
+ * For framebuffers allocated using vmalloc(), struct fb_info must have
+ * "screen_buffer" set to the vmalloc address of the framebuffer. For
+ * framebuffers allocated from contiguous kernel memory, FBINFO_KMEMFB must
+ * be set, and "fix.smem_start" must be set to the physical address of the
+ * frame buffer. In both cases, "fix.smem_len" must be set to the framebuffer
+ * size in bytes.
+ */
+
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/errno.h>
#include <linux/string.h>
#include <linux/mm.h>
+#include <linux/pfn_t.h>
#include <linux/vmalloc.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
@@ -37,7 +66,7 @@ static struct page *fb_deferred_io_get_page(struct fb_info *info, unsigned long
else if (info->fix.smem_start)
page = pfn_to_page((info->fix.smem_start + offs) >> PAGE_SHIFT);
- if (page)
+ if (page && !(info->flags & FBINFO_KMEMFB))
get_page(page);
return page;
@@ -137,6 +166,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
BUG_ON(!info->fbdefio->mapping);
+ if (info->flags & FBINFO_KMEMFB)
+ /*
+ * In this path, the VMA is marked VM_PFNMAP, so mm assumes
+ * there is no struct page associated with the page. The
+ * PFN must be directly inserted and the created PTE will be
+ * marked "special".
+ */
+ return vmf_insert_pfn(vmf->vma, vmf->address, page_to_pfn(page));
+
vmf->page = page;
return 0;
}
@@ -163,13 +201,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
/*
* Adds a page to the dirty list. Call this from struct
- * vm_operations_struct.page_mkwrite.
+ * vm_operations_struct.page_mkwrite or .pfn_mkwrite.
*/
-static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
+static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, struct vm_fault *vmf,
struct page *page)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
struct fb_deferred_io_pageref *pageref;
+ unsigned long offset = vmf->pgoff << PAGE_SHIFT;
vm_fault_t ret;
/* protect against the workqueue changing the page list */
@@ -182,20 +221,34 @@ static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long
}
/*
- * We want the page to remain locked from ->page_mkwrite until
- * the PTE is marked dirty to avoid mapping_wrprotect_range()
- * being called before the PTE is updated, which would leave
- * the page ignored by defio.
- * Do this by locking the page here and informing the caller
- * about it with VM_FAULT_LOCKED.
+ * The PTE must be marked writable before the defio deferred work runs
+ * again and potentially marks the PTE write-protected. If the order
+ * should be switched, the PTE would become writable without defio
+ * tracking the page, leaving the page forever ignored by defio.
+ *
+ * For vmalloc() framebuffers, the associated struct page is locked
+ * before releasing the defio lock. mm will later mark the PTE writaable
+ * and release the struct page lock. The struct page lock prevents
+ * the page from being prematurely being marked write-protected.
+ *
+ * For FBINFO_KMEMFB framebuffers, mm assumes there is no struct page,
+ * so the PTE must be marked writable while the defio lock is held.
*/
- lock_page(pageref->page);
+ if (info->flags & FBINFO_KMEMFB) {
+ unsigned long pfn = page_to_pfn(pageref->page);
+
+ ret = vmf_insert_mixed_mkwrite(vmf->vma, vmf->address,
+ __pfn_to_pfn_t(pfn, PFN_SPECIAL));
+ } else {
+ lock_page(pageref->page);
+ ret = VM_FAULT_LOCKED;
+ }
mutex_unlock(&fbdefio->lock);
/* come back after delay to process the deferred IO */
schedule_delayed_work(&info->deferred_work, fbdefio->delay);
- return VM_FAULT_LOCKED;
+ return ret;
err_mutex_unlock:
mutex_unlock(&fbdefio->lock);
@@ -207,10 +260,10 @@ static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long
* @fb_info: The fbdev info structure
* @vmf: The VM fault
*
- * This is a callback we get when userspace first tries to
- * write to the page. We schedule a workqueue. That workqueue
- * will eventually mkclean the touched pages and execute the
- * deferred framebuffer IO. Then if userspace touches a page
+ * This is a callback we get when userspace first tries to write to a
+ * page. We schedule a workqueue. That workqueue will eventually do
+ * mapping_wrprotect_range() on the written pages and execute the
+ * deferred framebuffer IO. Then if userspace writes to a page
* again, we repeat the same scheme.
*
* Returns:
@@ -218,12 +271,11 @@ static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long
*/
static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
{
- unsigned long offset = vmf->pgoff << PAGE_SHIFT;
struct page *page = vmf->page;
file_update_time(vmf->vma->vm_file);
- return fb_deferred_io_track_page(info, offset, page);
+ return fb_deferred_io_track_page(info, vmf, page);
}
/* vm_ops->page_mkwrite handler */
@@ -234,9 +286,25 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
return fb_deferred_io_page_mkwrite(info, vmf);
}
+/*
+ * Similar to fb_deferred_io_mkwrite(), but for first writes to pages
+ * in VMAs that have VM_PFNMAP set.
+ */
+static vm_fault_t fb_deferred_io_pfn_mkwrite(struct vm_fault *vmf)
+{
+ struct fb_info *info = vmf->vma->vm_private_data;
+ unsigned long offset = vmf->pgoff << PAGE_SHIFT;
+ struct page *page = phys_to_page(info->fix.smem_start + offset);
+
+ file_update_time(vmf->vma->vm_file);
+
+ return fb_deferred_io_track_page(info, vmf, page);
+}
+
static const struct vm_operations_struct fb_deferred_io_vm_ops = {
.fault = fb_deferred_io_fault,
.page_mkwrite = fb_deferred_io_mkwrite,
+ .pfn_mkwrite = fb_deferred_io_pfn_mkwrite,
};
static const struct address_space_operations fb_deferred_io_aops = {
@@ -246,11 +314,31 @@ static const struct address_space_operations fb_deferred_io_aops = {
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+ vm_flags_t flags = VM_DONTEXPAND | VM_DONTDUMP;
vma->vm_ops = &fb_deferred_io_vm_ops;
- vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP);
- if (!(info->flags & FBINFO_VIRTFB))
- vm_flags_set(vma, VM_IO);
+ if (info->flags & FBINFO_KMEMFB) {
+ /*
+ * I/O fault path calls vmf_insert_pfn(), which bug checks
+ * if the vma is not marked shared. mmap'ing the framebuffer
+ * as PRIVATE doesn't really make sense anyway, though doing
+ * so isn't harmful for vmalloc() framebuffers. So there's
+ * no prohibition for that case.
+ */
+ if (!(vma->vm_flags & VM_SHARED))
+ return -EINVAL;
+ /*
+ * Set VM_PFNMAP so mm code will not try to manage the pages'
+ * lifecycles. We don't want individual pages to be freed
+ * based on refcount. Instead the memory must be returned to
+ * the free pool in the usual way. Cf. the implementation of
+ * remap_pfn_range() and remap_pfn_range_internal().
+ */
+ flags |= VM_PFNMAP | VM_IO;
+ } else if (!(info->flags & FBINFO_VIRTFB)) {
+ flags |= VM_IO;
+ }
+ vm_flags_set(vma, flags);
vma->vm_private_data = info;
return 0;
}
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 05cc251035da..a1121116eef0 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -396,6 +396,7 @@ struct fb_tile_ops {
/* hints */
#define FBINFO_VIRTFB 0x0004 /* FB is System RAM, not device. */
+#define FBINFO_KMEMFB 0x0008 /* FB is allocated in contig kernel mem */
#define FBINFO_PARTIAL_PAN_OK 0x0040 /* otw use pan only for double-buffering */
#define FBINFO_READS_FAST 0x0080 /* soft-copy faster than rendering */
--
2.25.1
^ permalink raw reply related
* [PATCH v2 1/3] mm: Export vmf_insert_mixed_mkwrite()
From: mhkelley58 @ 2025-05-02 4:05 UTC (permalink / raw)
To: simona, deller, haiyangz, kys, wei.liu, decui, akpm
Cc: weh, tzimmermann, hch, dri-devel, linux-fbdev, linux-kernel,
linux-hyperv, linux-mm
In-Reply-To: <20250502040525.822075-1-mhklinux@outlook.com>
From: Michael Kelley <mhklinux@outlook.com>
Export vmf_insert_mixed_mkwrite() for use by fbdev deferred I/O code,
which can be built as a module.
Commit cd1e0dac3a3e ("mm: unexport vmf_insert_mixed_mkwrite") is
effectively reverted.
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
Changes in v2:
* Exported as GPL symbol [Christoph Hellwig]
mm/memory.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/memory.c b/mm/memory.c
index 424420349bd3..24dc2aacea62 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2698,6 +2698,7 @@ vm_fault_t vmf_insert_mixed_mkwrite(struct vm_area_struct *vma,
{
return __vm_insert_mixed(vma, addr, pfn, true);
}
+EXPORT_SYMBOL_GPL(vmf_insert_mixed_mkwrite);
/*
* maps a range of physical memory into the requested pages. the old
--
2.25.1
^ permalink raw reply related
* [PATCH v2 0/3] fbdev: Add deferred I/O support for contiguous kernel memory framebuffers
From: mhkelley58 @ 2025-05-02 4:05 UTC (permalink / raw)
To: simona, deller, haiyangz, kys, wei.liu, decui, akpm
Cc: weh, tzimmermann, hch, dri-devel, linux-fbdev, linux-kernel,
linux-hyperv, linux-mm
From: Michael Kelley <mhklinux@outlook.com>
Current deferred I/O code works only for framebuffer memory that is
allocated with vmalloc(). The code assumes that the underlying page
refcount can be used by the mm subsystem to manage each framebuffer
page's lifecycle, which is consistent with vmalloc'ed memory, but not
with contiguous kernel memory from alloc_pages() or similar. When used
with contiguous kernel memory, current deferred I/O code eventually
causes the memory free lists to be scrambled, and a kernel panic ensues.
The problem is seen with the hyperv_fb driver when mmap'ing the
framebuffer into user space, as that driver uses alloc_pages() for the
framebuffer in some configurations. This patch set fixes the problem
by supporting contiguous kernel memory framebuffers with deferred I/O.
Patch 1 exports a 'mm' subsystem function needed by Patch 2.
Patch 2 is the changes to the fbdev deferred I/O code. More details
are in the commit message of Patch 2.
Patch 3 updates the hyperv_fb driver to use the new functionality
from Patch 2.
Michael Kelley (3):
mm: Export vmf_insert_mixed_mkwrite()
fbdev/deferred-io: Support contiguous kernel memory framebuffers
fbdev: hyperv_fb: Fix mmap of framebuffers allocated using
alloc_pages()
drivers/video/fbdev/core/fb_defio.c | 128 +++++++++++++++++++++++-----
drivers/video/fbdev/hyperv_fb.c | 1 +
include/linux/fb.h | 1 +
mm/memory.c | 1 +
4 files changed, 111 insertions(+), 20 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Uwe Kleine-König @ 2025-04-30 12:33 UTC (permalink / raw)
To: Abel Vesa
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bjorn Andersson, Konrad Dybcio, Johan Hovold, Sebastian Reichel,
linux-pwm, dri-devel, linux-arm-msm, linux-fbdev, linux-kernel
In-Reply-To: <Z8CX3vr1xuaKT38m@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 4920 bytes --]
Hello Abel,
On Thu, Feb 27, 2025 at 06:50:38PM +0200, Abel Vesa wrote:
> On 25-02-27 16:51:15, Uwe Kleine-König wrote:
> > On Thu, Feb 27, 2025 at 03:07:21PM +0200, Abel Vesa wrote:
> > > On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> > > > On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > > > > The current implementation assumes that the PWM provider will be able to
> > > > > meet the requested period, but that is not always the case. Some PWM
> > > > > providers have limited HW configuration capabilities and can only
> > > > > provide a period that is somewhat close to the requested one. This
> > > > > simply means that the duty cycle requested might either be above the
> > > > > PWM's maximum value or the 100% duty cycle is never reached.
> > > >
> > > > If you request a state with 100% relative duty cycle you should get 100%
> > > > unless the hardware cannot do that. Which PWM hardware are you using?
> > > > Which requests are you actually doing that don't match your expectation?
> > >
> > > The PWM hardware is Qualcomm PMK8550 PMIC. The way the duty cycle is
> > > controlled is described in the following comment found in lpg_calc_freq
> > > of the leds-qcom-lpg driver:
> > >
> > > /*
> > > * The PWM period is determined by:
> > > *
> > > * resolution * pre_div * 2^M
> > > * period = --------------------------
> > > * refclk
> > > *
> > > * Resolution = 2^9 bits for PWM or
> > > * 2^{8, 9, 10, 11, 12, 13, 14, 15} bits for high resolution PWM
> > > * pre_div = {1, 3, 5, 6} and
> > > * M = [0..7].
> > > *
> > > * This allows for periods between 27uS and 384s for PWM channels and periods between
> > > * 3uS and 24576s for high resolution PWMs.
> > > * The PWM framework wants a period of equal or lower length than requested,
> > > * reject anything below minimum period.
> > > */
> > >
> > > So if we request a period of 5MHz, that will not ever be reached no matter what config
> > > is used. Instead, the 4.26 MHz is selected as closest possible.
> >
> > The trace in the other mail thread suggest that you asked for a period
> > of 5 ms, not 5 MHz. And that results in a period of 4.26 ms.
>
> OK. So unit is ms. Got it.
>
> >
> > > Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
> > > go above 4.26MHz.
> >
> > It requests .period = 5 ms + .duty_cycle = 5 ms. This is fine, and
> > according to the trace this results in both values becoming 4.26 ms in
> > real life. Seems fine to me.
>
> Right, but as I keep trying to explain is that, the consumer keeps
> asking for duty cycles that go over the 4.26ms, which is the period that
> the provider decided it can do instead of 5ms.
I see no problem here. Yes, requests are not implemented exactly in
general, but there is no way around that. For some use-cases the picked
configuration is sensible, for others less so. There is also no way
around that.
> > > > > This could be easily fixed if the pwm_apply*() API family would allow
> > > > > overriding the period within the PWM state that's used for providing the
> > > > > duty cycle. But that is currently not the case.
> > > >
> > > > I don't understand what you mean here.
> > >
> > > What I was trying to say is that the PWM generic framework currently doesn't
> > > allow overriding the PWM state's period with one provided by the consumer,
> > > when calling pwm_apply_might_sleep().
> >
> > Either I still don't understand what you want, or that is impossible or
> > useless. If you target .period = 5 ms and the hardware can only do 4.26
> > ms, why would you want to override period to 5 ms?
>
> Meaning the consumer should become aware of the period the provider can
> do before asking for a duty cycle.
There is pwm_round_waveform_might_sleep() that you could use. Downside
is that is currently only works for two lowlevel drivers.
> If you look at the other mail thread, the trace there shows the
> following sequence for every new backlight update request:
>
> 1. pwm_apply with consumer's period (5ms)
> 2. pwm_get reads the provider's period (4.25ms)
> - which is what the provider is able to do instead of 5ms
> 3. pwm_apply (due to debug) which uses the state from 2.
> 4. pwm_get reads back exactly as 2.
>
> So we can ignore 3 and 4 for now as they are there due to debug,
> but the step 1 still requests a value over the 4.26ms (5ms),
> which in the provider will translate to a pwm value higher than allowed
> by the selected configuration.
The lowlevel driver sees the request e.g.
.period = 5000
.duty_cycle = 4600
and is supposed to implement (I guess)
.period = 4260
.duty_cycle = 4260
. I don't see a technical problem here (apart from you being unlucky
about the choice made?).
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH RFC] backlight: pwm_bl: Read back PWM period from provider
From: Uwe Kleine-König @ 2025-04-30 12:25 UTC (permalink / raw)
To: Daniel Thompson
Cc: Sebastian Reichel, Abel Vesa, Lee Jones, Jingoo Han, Helge Deller,
Bjorn Andersson, Konrad Dybcio, Johan Hovold, linux-pwm,
dri-devel, linux-arm-msm, linux-fbdev, linux-kernel
In-Reply-To: <Z9lFg98srzYivGoI@aspen.lan>
[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]
Hello Daniel,
On Tue, Mar 18, 2025 at 10:05:55AM +0000, Daniel Thompson wrote:
> On Thu, Feb 27, 2025 at 04:06:47AM +0100, Sebastian Reichel wrote:
> > On Wed, Feb 26, 2025 at 05:34:50PM +0100, Uwe Kleine-König wrote:
> > > On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > > > The current implementation assumes that the PWM provider will be able to
> > > > meet the requested period, but that is not always the case. Some PWM
> > > > providers have limited HW configuration capabilities and can only
> > > > provide a period that is somewhat close to the requested one. This
> > > > simply means that the duty cycle requested might either be above the
> > > > PWM's maximum value or the 100% duty cycle is never reached.
> > >
> > > If you request a state with 100% relative duty cycle you should get 100%
> > > unless the hardware cannot do that. Which PWM hardware are you using?
> > > Which requests are you actually doing that don't match your expectation?
> >
> > drivers/leds/rgb/leds-qcom-lpg.c (which probably should at least get
> > a MAINTAINERS entry to have you CC'd considering all the PWM bits in
> > it). See the following discussion (I point you to my message in the
> > middle of a thread, which has a summary and probably is a good
> > starting point):
> >
> > https://lore.kernel.org/all/vc7irlp7nuy5yvkxwb5m7wy7j7jzgpg73zmajbmq2zjcd67pd2@cz2dcracta6w/
>
> I had a quick glance at this thread.
>
> It sounded to me like the PWM driver was scaling the requested period
> to match h/ware capability but then neglected to scale the requested
> duty cycle accordingly.
Well, I'd not call the period adaption "scaling", it just gets fitted to
the hardware capabilities. The same happens for duty_cycle, it's just
that the absolute duty_cycle value is reduced to the next value that is
possible to implement. Obviously that modifies the ratio between
duty_cycle and period (requested vs. implemented), but you cannot
prevent that anyhow and it makes handling easier for the lowlevel driver
with less corner cases. And whatever policy is chosen to be the right
one, it becomes ridiculous in the corner cases, so picking the simplest
to implement is the sane option in my eyes.
> That means the qcomm PWM driver programming a
> fractional value into the hardware that was not being anywhere close
> to duty_cycle / period.
>
> So the recommendation was to fix the PWM driver rather than have
> pwm_bl.c work around it?
No, the lowlevel driver is fine.
With the new-style driver callbacks it becomes possible to query the
hardware capabilities enough to implement a helper that determines the
actually implementable waveform that is best for your use-case, whatever
"best" means here.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH 5/5] staging: sm750fb: rename sm750_hw_cursor_setData2
From: Eric Florin @ 2025-04-30 5:55 UTC (permalink / raw)
To: teddy.wang
Cc: sudipm.mukherjee, gregkh, linux-fbdev, linux-staging,
linux-kernel, Eric Florin
In-Reply-To: <cover.1745982772.git.ericflorin.kernel@gmail.com>
Rename sm750_hw_cursor_setData2 to sm750_hw_cursor_set_data2 to conform
with kernel style guidelines as reported by checkpatch.pl
CHECK: Avoid CamelCase: <sm750_hw_cursor_setData2>
Signed-off-by: Eric Florin <ericflorin.kernel@gmail.com>
---
drivers/staging/sm750fb/sm750_cursor.c | 4 ++--
drivers/staging/sm750fb/sm750_cursor.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index 3aa26ef00011..7ede144905c9 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c
@@ -131,8 +131,8 @@ void sm750_hw_cursor_set_data(struct lynx_cursor *cursor, u16 rop,
}
}
-void sm750_hw_cursor_setData2(struct lynx_cursor *cursor, u16 rop,
- const u8 *pcol, const u8 *pmsk)
+void sm750_hw_cursor_set_data2(struct lynx_cursor *cursor, u16 rop,
+ const u8 *pcol, const u8 *pmsk)
{
int i, j, count, pitch, offset;
u8 color, mask;
diff --git a/drivers/staging/sm750fb/sm750_cursor.h b/drivers/staging/sm750fb/sm750_cursor.h
index cbb896a35160..88fa02f6377a 100644
--- a/drivers/staging/sm750fb/sm750_cursor.h
+++ b/drivers/staging/sm750fb/sm750_cursor.h
@@ -10,6 +10,6 @@ void sm750_hw_cursor_set_pos(struct lynx_cursor *cursor, int x, int y);
void sm750_hw_cursor_set_color(struct lynx_cursor *cursor, u32 fg, u32 bg);
void sm750_hw_cursor_set_data(struct lynx_cursor *cursor, u16 rop,
const u8 *data, const u8 *mask);
-void sm750_hw_cursor_setData2(struct lynx_cursor *cursor, u16 rop,
- const u8 *data, const u8 *mask);
+void sm750_hw_cursor_set_data2(struct lynx_cursor *cursor, u16 rop,
+ const u8 *data, const u8 *mask);
#endif
--
2.39.5
^ permalink raw reply related
* [PATCH 4/5] staging: sm750fb: rename sm750_hw_cursor_setData
From: Eric Florin @ 2025-04-30 5:55 UTC (permalink / raw)
To: teddy.wang
Cc: sudipm.mukherjee, gregkh, linux-fbdev, linux-staging,
linux-kernel, Eric Florin
In-Reply-To: <cover.1745982772.git.ericflorin.kernel@gmail.com>
Rename sm750_hw_cursor_setData to sm750_hw_cursor_set_data to conform
with kernel style guidelines as reported by checkpatch.pl
CHECK: Avoid CamelCase: <sm750_hw_cursor_setData>
Signed-off-by: Eric Florin <ericflorin.kernel@gmail.com>
---
drivers/staging/sm750fb/sm750.c | 6 ++----
drivers/staging/sm750fb/sm750_cursor.c | 4 ++--
drivers/staging/sm750fb/sm750_cursor.h | 4 ++--
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 483a30841c77..d74836fbdfa3 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -145,10 +145,8 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor)
}
if (fbcursor->set & (FB_CUR_SETSHAPE | FB_CUR_SETIMAGE)) {
- sm750_hw_cursor_setData(cursor,
- fbcursor->rop,
- fbcursor->image.data,
- fbcursor->mask);
+ sm750_hw_cursor_set_data(cursor, fbcursor->rop, fbcursor->image.data,
+ fbcursor->mask);
}
if (fbcursor->enable)
diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index e80d6efe0ab1..3aa26ef00011 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c
@@ -81,8 +81,8 @@ void sm750_hw_cursor_set_color(struct lynx_cursor *cursor, u32 fg, u32 bg)
poke32(HWC_COLOR_3, 0xffe0);
}
-void sm750_hw_cursor_setData(struct lynx_cursor *cursor, u16 rop,
- const u8 *pcol, const u8 *pmsk)
+void sm750_hw_cursor_set_data(struct lynx_cursor *cursor, u16 rop,
+ const u8 *pcol, const u8 *pmsk)
{
int i, j, count, pitch, offset;
u8 color, mask, opr;
diff --git a/drivers/staging/sm750fb/sm750_cursor.h b/drivers/staging/sm750fb/sm750_cursor.h
index edfa6a8202cd..cbb896a35160 100644
--- a/drivers/staging/sm750fb/sm750_cursor.h
+++ b/drivers/staging/sm750fb/sm750_cursor.h
@@ -8,8 +8,8 @@ void sm750_hw_cursor_disable(struct lynx_cursor *cursor);
void sm750_hw_cursor_set_size(struct lynx_cursor *cursor, int w, int h);
void sm750_hw_cursor_set_pos(struct lynx_cursor *cursor, int x, int y);
void sm750_hw_cursor_set_color(struct lynx_cursor *cursor, u32 fg, u32 bg);
-void sm750_hw_cursor_setData(struct lynx_cursor *cursor, u16 rop,
- const u8 *data, const u8 *mask);
+void sm750_hw_cursor_set_data(struct lynx_cursor *cursor, u16 rop,
+ const u8 *data, const u8 *mask);
void sm750_hw_cursor_setData2(struct lynx_cursor *cursor, u16 rop,
const u8 *data, const u8 *mask);
#endif
--
2.39.5
^ permalink raw reply related
* [PATCH 3/5] staging: sm750fb: rename sm750_hw_cursor_setColor
From: Eric Florin @ 2025-04-30 5:55 UTC (permalink / raw)
To: teddy.wang
Cc: sudipm.mukherjee, gregkh, linux-fbdev, linux-staging,
linux-kernel, Eric Florin
In-Reply-To: <cover.1745982772.git.ericflorin.kernel@gmail.com>
Rename sm750_hw_cursor_setColor to sm750_hw_cursor_set_color to conform
with kernel style guidelines as reported by checkpatch.pl
CHECK: Avoid CamelCase: <sm750_hw_cursor_setColor>
Signed-off-by: Eric Florin <ericflorin.kernel@gmail.com>
---
drivers/staging/sm750fb/sm750.c | 2 +-
drivers/staging/sm750fb/sm750_cursor.c | 2 +-
drivers/staging/sm750fb/sm750_cursor.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 8dd32aa6ac6e..483a30841c77 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -141,7 +141,7 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor)
((info->cmap.green[fbcursor->image.bg_color] & 0xfc00) >> 5) |
((info->cmap.blue[fbcursor->image.bg_color] & 0xf800) >> 11);
- sm750_hw_cursor_setColor(cursor, fg, bg);
+ sm750_hw_cursor_set_color(cursor, fg, bg);
}
if (fbcursor->set & (FB_CUR_SETSHAPE | FB_CUR_SETIMAGE)) {
diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index a6fe241e7748..e80d6efe0ab1 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c
@@ -72,7 +72,7 @@ void sm750_hw_cursor_set_pos(struct lynx_cursor *cursor, int x, int y)
poke32(HWC_LOCATION, reg);
}
-void sm750_hw_cursor_setColor(struct lynx_cursor *cursor, u32 fg, u32 bg)
+void sm750_hw_cursor_set_color(struct lynx_cursor *cursor, u32 fg, u32 bg)
{
u32 reg = (fg << HWC_COLOR_12_2_RGB565_SHIFT) &
HWC_COLOR_12_2_RGB565_MASK;
diff --git a/drivers/staging/sm750fb/sm750_cursor.h b/drivers/staging/sm750fb/sm750_cursor.h
index d0ade8e366f4..edfa6a8202cd 100644
--- a/drivers/staging/sm750fb/sm750_cursor.h
+++ b/drivers/staging/sm750fb/sm750_cursor.h
@@ -7,7 +7,7 @@ void sm750_hw_cursor_enable(struct lynx_cursor *cursor);
void sm750_hw_cursor_disable(struct lynx_cursor *cursor);
void sm750_hw_cursor_set_size(struct lynx_cursor *cursor, int w, int h);
void sm750_hw_cursor_set_pos(struct lynx_cursor *cursor, int x, int y);
-void sm750_hw_cursor_setColor(struct lynx_cursor *cursor, u32 fg, u32 bg);
+void sm750_hw_cursor_set_color(struct lynx_cursor *cursor, u32 fg, u32 bg);
void sm750_hw_cursor_setData(struct lynx_cursor *cursor, u16 rop,
const u8 *data, const u8 *mask);
void sm750_hw_cursor_setData2(struct lynx_cursor *cursor, u16 rop,
--
2.39.5
^ permalink raw reply related
* [PATCH 2/5] staging: sm750fb: rename sm750_hw_cursor_setPos
From: Eric Florin @ 2025-04-30 5:55 UTC (permalink / raw)
To: teddy.wang
Cc: sudipm.mukherjee, gregkh, linux-fbdev, linux-staging,
linux-kernel, Eric Florin
In-Reply-To: <cover.1745982772.git.ericflorin.kernel@gmail.com>
Rename sm750_hw_cursor_setPos to sm750_hw_cursor_set_pos to conform with
kernel style guidelines as reported by checkpatch.pl
CHECK: Avoid CamelCase: <sm750_hw_cursor_setPos>
Signed-off-by: Eric Florin <ericflorin.kernel@gmail.com>
---
drivers/staging/sm750fb/sm750.c | 2 +-
drivers/staging/sm750fb/sm750_cursor.c | 2 +-
drivers/staging/sm750fb/sm750_cursor.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 47c84331e3d9..8dd32aa6ac6e 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -125,7 +125,7 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor)
fbcursor->image.height);
if (fbcursor->set & FB_CUR_SETPOS)
- sm750_hw_cursor_setPos(cursor,
+ sm750_hw_cursor_set_pos(cursor,
fbcursor->image.dx - info->var.xoffset,
fbcursor->image.dy - info->var.yoffset);
diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index 3128ff3f4b70..a6fe241e7748 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c
@@ -63,7 +63,7 @@ void sm750_hw_cursor_set_size(struct lynx_cursor *cursor, int w, int h)
cursor->h = h;
}
-void sm750_hw_cursor_setPos(struct lynx_cursor *cursor, int x, int y)
+void sm750_hw_cursor_set_pos(struct lynx_cursor *cursor, int x, int y)
{
u32 reg;
diff --git a/drivers/staging/sm750fb/sm750_cursor.h b/drivers/staging/sm750fb/sm750_cursor.h
index edeed2ea4b04..d0ade8e366f4 100644
--- a/drivers/staging/sm750fb/sm750_cursor.h
+++ b/drivers/staging/sm750fb/sm750_cursor.h
@@ -6,7 +6,7 @@
void sm750_hw_cursor_enable(struct lynx_cursor *cursor);
void sm750_hw_cursor_disable(struct lynx_cursor *cursor);
void sm750_hw_cursor_set_size(struct lynx_cursor *cursor, int w, int h);
-void sm750_hw_cursor_setPos(struct lynx_cursor *cursor, int x, int y);
+void sm750_hw_cursor_set_pos(struct lynx_cursor *cursor, int x, int y);
void sm750_hw_cursor_setColor(struct lynx_cursor *cursor, u32 fg, u32 bg);
void sm750_hw_cursor_setData(struct lynx_cursor *cursor, u16 rop,
const u8 *data, const u8 *mask);
--
2.39.5
^ permalink raw reply related
* [PATCH 1/5] staging: sm750fb: rename sm750_hw_cursor_setSize
From: Eric Florin @ 2025-04-30 5:55 UTC (permalink / raw)
To: teddy.wang
Cc: sudipm.mukherjee, gregkh, linux-fbdev, linux-staging,
linux-kernel, Eric Florin
In-Reply-To: <cover.1745982772.git.ericflorin.kernel@gmail.com>
Rename sm750_hw_cursor_setSize to sm750_hw_cursor_set_size to conform to
kernel style guidelines as reported by checkpatch.pl
CHECK: Avoid CamelCase: <sm750_hw_cursor_setSize>
Signed-off-by: Eric Florin <ericflorin.kernel@gmail.com>
---
drivers/staging/sm750fb/sm750.c | 2 +-
drivers/staging/sm750fb/sm750_cursor.c | 2 +-
drivers/staging/sm750fb/sm750_cursor.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 04c1b32a22c5..47c84331e3d9 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -120,7 +120,7 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor)
sm750_hw_cursor_disable(cursor);
if (fbcursor->set & FB_CUR_SETSIZE)
- sm750_hw_cursor_setSize(cursor,
+ sm750_hw_cursor_set_size(cursor,
fbcursor->image.width,
fbcursor->image.height);
diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index eea4d1bd36ce..3128ff3f4b70 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c
@@ -57,7 +57,7 @@ void sm750_hw_cursor_disable(struct lynx_cursor *cursor)
poke32(HWC_ADDRESS, 0);
}
-void sm750_hw_cursor_setSize(struct lynx_cursor *cursor, int w, int h)
+void sm750_hw_cursor_set_size(struct lynx_cursor *cursor, int w, int h)
{
cursor->w = w;
cursor->h = h;
diff --git a/drivers/staging/sm750fb/sm750_cursor.h b/drivers/staging/sm750fb/sm750_cursor.h
index b59643dd61ed..edeed2ea4b04 100644
--- a/drivers/staging/sm750fb/sm750_cursor.h
+++ b/drivers/staging/sm750fb/sm750_cursor.h
@@ -5,7 +5,7 @@
/* hw_cursor_xxx works for voyager,718 and 750 */
void sm750_hw_cursor_enable(struct lynx_cursor *cursor);
void sm750_hw_cursor_disable(struct lynx_cursor *cursor);
-void sm750_hw_cursor_setSize(struct lynx_cursor *cursor, int w, int h);
+void sm750_hw_cursor_set_size(struct lynx_cursor *cursor, int w, int h);
void sm750_hw_cursor_setPos(struct lynx_cursor *cursor, int x, int y);
void sm750_hw_cursor_setColor(struct lynx_cursor *cursor, u32 fg, u32 bg);
void sm750_hw_cursor_setData(struct lynx_cursor *cursor, u16 rop,
--
2.39.5
^ permalink raw reply related
* [PATCH 0/5] staging: sm750fb: Style cleanup for sm750fb
From: Eric Florin @ 2025-04-30 5:55 UTC (permalink / raw)
To: teddy.wang
Cc: sudipm.mukherjee, gregkh, linux-fbdev, linux-staging,
linux-kernel, Eric Florin
This set of patches addresses a number of cleanups attributed to
`drivers/staging/sm750fb/sm750_cursor.h`.
Patch 1: Rename sm750_hw_cursor_setSize to sm750_hw_cursor_set_size
Patch 2: Rename sm750_hw_cursor_setPos to sm750_hw_cursor_set_pos
Patch 3: Rename sm750_hw_cursor_setColor to sm750_hw_cursor_set_color
Patch 4: Rename sm750_hw_cursor_setData to sm750_hw_cursor_set_data
Patch 5: Rename sm750_hw_cursor_setData2 to sm750_hw_cursor_set_data2
Eric Florin (5):
staging: sm750fb: rename sm750_hw_cursor_setSize
staging: sm750fb: rename sm750_hw_cursor_setPos
staging: sm750fb: rename sm750_hw_cursor_setColor
staging: sm750fb: rename sm750_hw_cursor_setData
staging: sm750fb: rename sm750_hw_cursor_setData2
drivers/staging/sm750fb/sm750.c | 12 +++++-------
drivers/staging/sm750fb/sm750_cursor.c | 14 +++++++-------
drivers/staging/sm750fb/sm750_cursor.h | 12 ++++++------
3 files changed, 18 insertions(+), 20 deletions(-)
--
2.39.5
^ permalink raw reply
* Re: [PATCH] video: fbdev: arkfb: Cast ics5342_init() allocation type
From: Kees Cook @ 2025-04-29 20:25 UTC (permalink / raw)
To: Helge Deller, Geert Uytterhoeven
Cc: Javier Martinez Canillas, Thomas Zimmermann, Zheyu Ma,
Samuel Thibault, Jiapeng Chong, linux-fbdev, dri-devel,
linux-kernel, linux-hardening
In-Reply-To: <e68c6218-6055-45a6-b96e-9c8381a4b409@gmx.de>
On April 29, 2025 1:17:26 PM PDT, Helge Deller <deller@gmx.de> wrote:
>On 4/28/25 08:36, Geert Uytterhoeven wrote:
>> Hi Kees,
>>
>> On Sat, 26 Apr 2025 at 13:33, Helge Deller <deller@gmx.de> wrote:
>>> On 4/26/25 08:23, Kees Cook wrote:
>>>> In preparation for making the kmalloc family of allocators type aware,
>>>> we need to make sure that the returned type from the allocation matches
>>>> the type of the variable being assigned. (Before, the allocator would
>>>> always return "void *", which can be implicitly cast to any pointer type.)
>>>>
>>>> The assigned type is "struct dac_info *" but the returned type will be
>>>> "struct ics5342_info *", which has a larger allocation size. This is
>>>> by design, as struct ics5342_info contains struct dac_info as its first
>>>> member. Cast the allocation type to match the assignment.
>>>>
>>>> Signed-off-by: Kees Cook <kees@kernel.org>
>>
>> Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
>> arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.
>>
>>> I applied your patch, but wouldn't this untested patch be cleaner and fulfill the
>>> same purpose to match a kzalloc return type?
>>>
>>> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
>>> index 7d131e3d159a..a57c8a992e11 100644
>>> --- a/drivers/video/fbdev/arkfb.c
>>> +++ b/drivers/video/fbdev/arkfb.c
>>> @@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
>>>
>>> static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
>>> {
>>> - struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
>>> + struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
>>
>> sizeof(*ics_info)?
>>
>>> + struct dac_info *info = &ics_info->dac;
>>
>> Exactly my thought when I noticed this commit. Adding casts makes
>> it harder to notice any future discrepancies.
>
>I've changed it accordingly.
Thanks! Yeah, that's a much nicer solution.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] video: fbdev: arkfb: Cast ics5342_init() allocation type
From: Helge Deller @ 2025-04-29 20:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Kees Cook
Cc: Javier Martinez Canillas, Thomas Zimmermann, Zheyu Ma,
Samuel Thibault, Jiapeng Chong, linux-fbdev, dri-devel,
linux-kernel, linux-hardening
In-Reply-To: <CAMuHMdVY1_gEqULGD0BzdTd05OAkodhk+RXKRAy-T-0+RJt7yQ@mail.gmail.com>
On 4/28/25 08:36, Geert Uytterhoeven wrote:
> Hi Kees,
>
> On Sat, 26 Apr 2025 at 13:33, Helge Deller <deller@gmx.de> wrote:
>> On 4/26/25 08:23, Kees Cook wrote:
>>> In preparation for making the kmalloc family of allocators type aware,
>>> we need to make sure that the returned type from the allocation matches
>>> the type of the variable being assigned. (Before, the allocator would
>>> always return "void *", which can be implicitly cast to any pointer type.)
>>>
>>> The assigned type is "struct dac_info *" but the returned type will be
>>> "struct ics5342_info *", which has a larger allocation size. This is
>>> by design, as struct ics5342_info contains struct dac_info as its first
>>> member. Cast the allocation type to match the assignment.
>>>
>>> Signed-off-by: Kees Cook <kees@kernel.org>
>
> Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
> arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.
>
>> I applied your patch, but wouldn't this untested patch be cleaner and fulfill the
>> same purpose to match a kzalloc return type?
>>
>> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
>> index 7d131e3d159a..a57c8a992e11 100644
>> --- a/drivers/video/fbdev/arkfb.c
>> +++ b/drivers/video/fbdev/arkfb.c
>> @@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
>>
>> static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
>> {
>> - struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
>> + struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
>
> sizeof(*ics_info)?
>
>> + struct dac_info *info = &ics_info->dac;
>
> Exactly my thought when I noticed this commit. Adding casts makes
> it harder to notice any future discrepancies.
I've changed it accordingly.
Helge
^ permalink raw reply
* [PATCH 2/2] fbdev: Fix fb_ser_var to prevent null-ptr-deref in fb_videomode_to_var
From: Murad Masimov @ 2025-04-28 15:34 UTC (permalink / raw)
To: Simona Vetter
Cc: Helge Deller, Murad Masimov, Thomas Weißschuh, linux-fbdev,
dri-devel, linux-kernel, lvc-project, stable
In-Reply-To: <20250428153407.3743416-1-m.masimov@mt-integration.ru>
If fb_add_videomode() in fb_set_var() fails to allocate memory for
fb_videomode, later it may lead to a null-ptr dereference in
fb_videomode_to_var(), as the fb_info is registered while not having the
mode in modelist that is expected to be there, i.e. the one that is
described in fb_info->var.
================================================================
general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN NOPTI
KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
CPU: 1 PID: 30371 Comm: syz-executor.1 Not tainted 5.10.226-syzkaller #0
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
RIP: 0010:fb_videomode_to_var+0x24/0x610 drivers/video/fbdev/core/modedb.c:901
Call Trace:
display_to_var+0x3a/0x7c0 drivers/video/fbdev/core/fbcon.c:929
fbcon_resize+0x3e2/0x8f0 drivers/video/fbdev/core/fbcon.c:2071
resize_screen drivers/tty/vt/vt.c:1176 [inline]
vc_do_resize+0x53a/0x1170 drivers/tty/vt/vt.c:1263
fbcon_modechanged+0x3ac/0x6e0 drivers/video/fbdev/core/fbcon.c:2720
fbcon_update_vcs+0x43/0x60 drivers/video/fbdev/core/fbcon.c:2776
do_fb_ioctl+0x6d2/0x740 drivers/video/fbdev/core/fbmem.c:1128
fb_ioctl+0xe7/0x150 drivers/video/fbdev/core/fbmem.c:1203
vfs_ioctl fs/ioctl.c:48 [inline]
__do_sys_ioctl fs/ioctl.c:753 [inline]
__se_sys_ioctl fs/ioctl.c:739 [inline]
__x64_sys_ioctl+0x19a/0x210 fs/ioctl.c:739
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x67/0xd1
================================================================
The reason is that fb_info->var is being modified in fb_set_var(), and
then fb_videomode_to_var() is called. If it fails to add the mode to
fb_info->modelist, fb_set_var() returns error, but does not restore the
old value of fb_info->var. Restore fb_info->var on fail the same way it
is done earlier in the function.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Murad Masimov <m.masimov@mt-integration.ru>
---
drivers/video/fbdev/core/fbmem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index e1557d80768f..eca2498f2436 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -328,8 +328,10 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
!list_empty(&info->modelist))
ret = fb_add_videomode(&mode, &info->modelist);
- if (ret)
+ if (ret) {
+ info->var = old_var;
return ret;
+ }
event.info = info;
event.data = &mode;
--
2.39.2
^ permalink raw reply related
* [PATCH 1/2] fbdev: Fix do_register_framebuffer to prevent null-ptr-deref in fb_videomode_to_var
From: Murad Masimov @ 2025-04-28 15:34 UTC (permalink / raw)
To: Simona Vetter
Cc: Helge Deller, Murad Masimov, Thomas Weißschuh, linux-fbdev,
dri-devel, linux-kernel, lvc-project, stable
In-Reply-To: <20250428153407.3743416-1-m.masimov@mt-integration.ru>
If fb_add_videomode() in do_register_framebuffer() fails to allocate
memory for fb_videomode, it will later lead to a null-ptr dereference in
fb_videomode_to_var(), as the fb_info is registered while not having the
mode in modelist that is expected to be there, i.e. the one that is
described in fb_info->var.
================================================================
general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN NOPTI
KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
CPU: 1 PID: 30371 Comm: syz-executor.1 Not tainted 5.10.226-syzkaller #0
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
RIP: 0010:fb_videomode_to_var+0x24/0x610 drivers/video/fbdev/core/modedb.c:901
Call Trace:
display_to_var+0x3a/0x7c0 drivers/video/fbdev/core/fbcon.c:929
fbcon_resize+0x3e2/0x8f0 drivers/video/fbdev/core/fbcon.c:2071
resize_screen drivers/tty/vt/vt.c:1176 [inline]
vc_do_resize+0x53a/0x1170 drivers/tty/vt/vt.c:1263
fbcon_modechanged+0x3ac/0x6e0 drivers/video/fbdev/core/fbcon.c:2720
fbcon_update_vcs+0x43/0x60 drivers/video/fbdev/core/fbcon.c:2776
do_fb_ioctl+0x6d2/0x740 drivers/video/fbdev/core/fbmem.c:1128
fb_ioctl+0xe7/0x150 drivers/video/fbdev/core/fbmem.c:1203
vfs_ioctl fs/ioctl.c:48 [inline]
__do_sys_ioctl fs/ioctl.c:753 [inline]
__se_sys_ioctl fs/ioctl.c:739 [inline]
__x64_sys_ioctl+0x19a/0x210 fs/ioctl.c:739
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x67/0xd1
================================================================
Even though fbcon_init() checks beforehand if fb_match_mode() in
var_to_display() fails, it can not prevent the panic because fbcon_init()
does not return error code. Considering this and the comment in the code
about fb_match_mode() returning NULL - "This should not happen" - it is
better to prevent registering the fb_info if its mode was not set
successfully. Also move fb_add_videomode() closer to the beginning of
do_register_framebuffer() to avoid having to do the cleanup on fail.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Murad Masimov <m.masimov@mt-integration.ru>
---
drivers/video/fbdev/core/fbmem.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3c568cff2913..e1557d80768f 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -388,7 +388,7 @@ static int fb_check_foreignness(struct fb_info *fi)
static int do_register_framebuffer(struct fb_info *fb_info)
{
- int i;
+ int i, err = 0;
struct fb_videomode mode;
if (fb_check_foreignness(fb_info))
@@ -397,10 +397,18 @@ static int do_register_framebuffer(struct fb_info *fb_info)
if (num_registered_fb == FB_MAX)
return -ENXIO;
- num_registered_fb++;
for (i = 0 ; i < FB_MAX; i++)
if (!registered_fb[i])
break;
+
+ if (!fb_info->modelist.prev || !fb_info->modelist.next)
+ INIT_LIST_HEAD(&fb_info->modelist);
+
+ fb_var_to_videomode(&mode, &fb_info->var);
+ err = fb_add_videomode(&mode, &fb_info->modelist);
+ if (err < 0)
+ return err;
+
fb_info->node = i;
refcount_set(&fb_info->count, 1);
mutex_init(&fb_info->lock);
@@ -426,16 +434,12 @@ static int do_register_framebuffer(struct fb_info *fb_info)
if (bitmap_empty(fb_info->pixmap.blit_y, FB_MAX_BLIT_HEIGHT))
bitmap_fill(fb_info->pixmap.blit_y, FB_MAX_BLIT_HEIGHT);
- if (!fb_info->modelist.prev || !fb_info->modelist.next)
- INIT_LIST_HEAD(&fb_info->modelist);
-
if (fb_info->skip_vt_switch)
pm_vt_switch_required(fb_info->device, false);
else
pm_vt_switch_required(fb_info->device, true);
- fb_var_to_videomode(&mode, &fb_info->var);
- fb_add_videomode(&mode, &fb_info->modelist);
+ num_registered_fb++;
registered_fb[i] = fb_info;
#ifdef CONFIG_GUMSTIX_AM200EPD
--
2.39.2
^ permalink raw reply related
* [PATCH 0/2] fbdev: Prevent null-ptr-deref in fb_videomode_to_var
From: Murad Masimov @ 2025-04-28 15:34 UTC (permalink / raw)
To: Simona Vetter
Cc: Helge Deller, Murad Masimov, Thomas Weißschuh, linux-fbdev,
dri-devel, linux-kernel, lvc-project
These patches fix the bug that leads to a null-ptr-deref if
fb_videomode_to_var() fails to allocate memory. This bug is present in
do_register_framebuffer() and fb_ser_var().
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Murad Masimov (2):
fbdev: Fix do_register_framebuffer to prevent null-ptr-deref in
fb_videomode_to_var
fbdev: Fix fb_ser_var to prevent null-ptr-deref in fb_videomode_to_var
drivers/video/fbdev/core/fbmem.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
--
2.39.2
^ permalink raw reply
* Re: [PATCH] video: fbdev: arkfb: Cast ics5342_init() allocation type
From: Geert Uytterhoeven @ 2025-04-28 6:36 UTC (permalink / raw)
To: Kees Cook
Cc: Javier Martinez Canillas, Thomas Zimmermann, Zheyu Ma,
Samuel Thibault, Jiapeng Chong, linux-fbdev, dri-devel,
linux-kernel, linux-hardening, Helge Deller
In-Reply-To: <b982d4f1-6ed8-490b-8d47-6dc5231913e7@gmx.de>
Hi Kees,
On Sat, 26 Apr 2025 at 13:33, Helge Deller <deller@gmx.de> wrote:
> On 4/26/25 08:23, Kees Cook wrote:
> > In preparation for making the kmalloc family of allocators type aware,
> > we need to make sure that the returned type from the allocation matches
> > the type of the variable being assigned. (Before, the allocator would
> > always return "void *", which can be implicitly cast to any pointer type.)
> >
> > The assigned type is "struct dac_info *" but the returned type will be
> > "struct ics5342_info *", which has a larger allocation size. This is
> > by design, as struct ics5342_info contains struct dac_info as its first
> > member. Cast the allocation type to match the assignment.
> >
> > Signed-off-by: Kees Cook <kees@kernel.org>
Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.
> I applied your patch, but wouldn't this untested patch be cleaner and fulfill the
> same purpose to match a kzalloc return type?
>
> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> index 7d131e3d159a..a57c8a992e11 100644
> --- a/drivers/video/fbdev/arkfb.c
> +++ b/drivers/video/fbdev/arkfb.c
> @@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
>
> static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
> {
> - struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
> + struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
sizeof(*ics_info)?
> + struct dac_info *info = &ics_info->dac;
Exactly my thought when I noticed this commit. Adding casts makes
it harder to notice any future discrepancies.
> > --- a/drivers/video/fbdev/arkfb.c
> > +++ b/drivers/video/fbdev/arkfb.c
> > @@ -431,7 +431,7 @@ static struct dac_ops ics5342_ops = {
> >
> > static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
> > {
> > - struct dac_info *info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
> > + struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
> >
> > if (! info)
> > return NULL;
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
^ permalink raw reply
* Re: [PATCH RFC 1/1] vgacon: Add check for vc_origin address range in vgacon_scroll()
From: Greg Kroah-Hartman @ 2025-04-27 15:40 UTC (permalink / raw)
To: Helge Deller
Cc: GONG Ruiqi, Jiri Slaby, linux-fbdev, security, Kees Cook, Yi Yang,
Lu Jialin, Xiu Jianfeng
In-Reply-To: <bd358b87-8bca-4c45-9ee2-43d8d106969f@gmx.de>
On Sun, Apr 27, 2025 at 05:34:47PM +0200, Helge Deller wrote:
> On 4/27/25 04:53, GONG Ruiqi wrote:
> > Our in-house Syzkaller reported the following BUG (twice), which we
> > believed was the same issue with [1]:
> >
> > ==================================================================
> > BUG: KASAN: slab-out-of-bounds in vcs_scr_readw+0xc2/0xd0 drivers/tty/vt/vt.c:4740
> > Read of size 2 at addr ffff88800f5bef60 by task syz.7.2620/12393
> > ...
> > Call Trace:
> > <TASK>
> > __dump_stack lib/dump_stack.c:88 [inline]
> > dump_stack_lvl+0x72/0xa0 lib/dump_stack.c:106
> > print_address_description.constprop.0+0x6b/0x3d0 mm/kasan/report.c:364
> > print_report+0xba/0x280 mm/kasan/report.c:475
> > kasan_report+0xa9/0xe0 mm/kasan/report.c:588
> > vcs_scr_readw+0xc2/0xd0 drivers/tty/vt/vt.c:4740
> > vcs_write_buf_noattr drivers/tty/vt/vc_screen.c:493 [inline]
> > vcs_write+0x586/0x840 drivers/tty/vt/vc_screen.c:690
> > vfs_write+0x219/0x960 fs/read_write.c:584
> > ksys_write+0x12e/0x260 fs/read_write.c:639
> > do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> > do_syscall_64+0x59/0x110 arch/x86/entry/common.c:81
> > entry_SYSCALL_64_after_hwframe+0x78/0xe2
> > ...
> > </TASK>
> >
> > Allocated by task 5614:
> > kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
> > kasan_set_track+0x25/0x30 mm/kasan/common.c:52
> > ____kasan_kmalloc mm/kasan/common.c:374 [inline]
> > __kasan_kmalloc+0x8f/0xa0 mm/kasan/common.c:383
> > kasan_kmalloc include/linux/kasan.h:201 [inline]
> > __do_kmalloc_node mm/slab_common.c:1007 [inline]
> > __kmalloc+0x62/0x140 mm/slab_common.c:1020
> > kmalloc include/linux/slab.h:604 [inline]
> > kzalloc include/linux/slab.h:721 [inline]
> > vc_do_resize+0x235/0xf40 drivers/tty/vt/vt.c:1193
> > vgacon_adjust_height+0x2d4/0x350 drivers/video/console/vgacon.c:1007
> > vgacon_font_set+0x1f7/0x240 drivers/video/console/vgacon.c:1031
> > con_font_set drivers/tty/vt/vt.c:4628 [inline]
> > con_font_op+0x4da/0xa20 drivers/tty/vt/vt.c:4675
> > vt_k_ioctl+0xa10/0xb30 drivers/tty/vt/vt_ioctl.c:474
> > vt_ioctl+0x14c/0x1870 drivers/tty/vt/vt_ioctl.c:752
> > tty_ioctl+0x655/0x1510 drivers/tty/tty_io.c:2779
> > vfs_ioctl fs/ioctl.c:51 [inline]
> > __do_sys_ioctl fs/ioctl.c:871 [inline]
> > __se_sys_ioctl+0x12d/0x190 fs/ioctl.c:857
> > do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> > do_syscall_64+0x59/0x110 arch/x86/entry/common.c:81
> > entry_SYSCALL_64_after_hwframe+0x78/0xe2
> >
> > Last potentially related work creation:
> > kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
> > __kasan_record_aux_stack+0x94/0xa0 mm/kasan/generic.c:492
> > __call_rcu_common.constprop.0+0xc3/0xa10 kernel/rcu/tree.c:2713
> > netlink_release+0x620/0xc20 net/netlink/af_netlink.c:802
> > __sock_release+0xb5/0x270 net/socket.c:663
> > sock_close+0x1e/0x30 net/socket.c:1425
> > __fput+0x408/0xab0 fs/file_table.c:384
> > __fput_sync+0x4c/0x60 fs/file_table.c:465
> > __do_sys_close fs/open.c:1580 [inline]
> > __se_sys_close+0x68/0xd0 fs/open.c:1565
> > do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> > do_syscall_64+0x59/0x110 arch/x86/entry/common.c:81
> > entry_SYSCALL_64_after_hwframe+0x78/0xe2
> >
> > Second to last potentially related work creation:
> > kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
> > __kasan_record_aux_stack+0x94/0xa0 mm/kasan/generic.c:492
> > __call_rcu_common.constprop.0+0xc3/0xa10 kernel/rcu/tree.c:2713
> > netlink_release+0x620/0xc20 net/netlink/af_netlink.c:802
> > __sock_release+0xb5/0x270 net/socket.c:663
> > sock_close+0x1e/0x30 net/socket.c:1425
> > __fput+0x408/0xab0 fs/file_table.c:384
> > task_work_run+0x154/0x240 kernel/task_work.c:239
> > exit_task_work include/linux/task_work.h:45 [inline]
> > do_exit+0x8e5/0x1320 kernel/exit.c:874
> > do_group_exit+0xcd/0x280 kernel/exit.c:1023
> > get_signal+0x1675/0x1850 kernel/signal.c:2905
> > arch_do_signal_or_restart+0x80/0x3b0 arch/x86/kernel/signal.c:310
> > exit_to_user_mode_loop kernel/entry/common.c:111 [inline]
> > exit_to_user_mode_prepare include/linux/entry-common.h:328 [inline]
> > __syscall_exit_to_user_mode_work kernel/entry/common.c:207 [inline]
> > syscall_exit_to_user_mode+0x1b3/0x1e0 kernel/entry/common.c:218
> > do_syscall_64+0x66/0x110 arch/x86/entry/common.c:87
> > entry_SYSCALL_64_after_hwframe+0x78/0xe2
> >
> > The buggy address belongs to the object at ffff88800f5be000
> > which belongs to the cache kmalloc-2k of size 2048
> > The buggy address is located 2656 bytes to the right of
> > allocated 1280-byte region [ffff88800f5be000, ffff88800f5be500)
> >
> > ...
> >
> > Memory state around the buggy address:
> > ffff88800f5bee00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ffff88800f5bee80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > > ffff88800f5bef00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ^
> > ffff88800f5bef80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ffff88800f5bf000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ==================================================================
> >
> > By analyzing the vmcore, we found that vc->vc_origin was somehow placed
> > one line prior to vc->vc_screenbuf when vc was in KD_TEXT mode, and
> > further writings to /dev/vcs caused out-of-bounds reads (and writes
> > right after) in vcs_write_buf_noattr().
> >
> > Our further experiments show that in most cases, vc->vc_origin equals to
> > vga_vram_base when the console is in KD_TEXT mode, and it's around
> > vc->vc_screenbuf for the KD_GRAPHICS mode. But via triggerring a
> > TIOCL_SETVESABLANK ioctl beforehand, we can make vc->vc_origin be around
> > vc->vc_screenbuf while the console is in KD_TEXT mode, and then by
> > writing the special 'ESC M' control sequence to the tty certain times
> > (depends on the value of `vc->state.y - vc->vc_top`), we can eventually
> > move vc->vc_origin prior to vc->vc_screenbuf. Here's the PoC, tested on
> > QEMU:
> >
> > ```
> > int main() {
> > const int RI_NUM = 10; // should be greater than `vc->state.y - vc->vc_top`
> > int tty_fd, vcs_fd;
> > const char *tty_path = "/dev/tty0";
> > const char *vcs_path = "/dev/vcs";
> > const char escape_seq[] = "\x1bM"; // ESC + M
> > const char trigger_seq[] = "Let's trigger an OOB write.";
> > struct vt_sizes vt_size = { 70, 2 };
> > int blank = TIOCL_BLANKSCREEN;
> >
> > tty_fd = open(tty_path, O_RDWR);
> >
> > char vesa_mode[] = { TIOCL_SETVESABLANK, 1 };
> > ioctl(tty_fd, TIOCLINUX, vesa_mode);
> >
> > ioctl(tty_fd, TIOCLINUX, &blank);
> > ioctl(tty_fd, VT_RESIZE, &vt_size);
> >
> > for (int i = 0; i < RI_NUM; ++i)
> > write(tty_fd, escape_seq, sizeof(escape_seq) - 1);
> >
> > vcs_fd = open(vcs_path, O_RDWR);
> > write(vcs_fd, trigger_seq, sizeof(trigger_seq));
> >
> > close(vcs_fd);
> > close(tty_fd);
> > return 0;
> > }
> > ```
> >
> > To solve this problem, add an address range validation check in
> > vgacon_scroll(), ensuring vc->vc_origin never precedes vc_screenbuf.
> >
> > Reported-by: syzbot+9c09fda97a1a65ea859b@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=9c09fda97a1a65ea859b [1]
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > Co-developed-by: Yi Yang <yiyang13@huawei.com>
> > Signed-off-by: Yi Yang <yiyang13@huawei.com>
> > Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>
> > ---
> > drivers/video/console/vgacon.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> > index 37bd18730fe0..f9cdbf8c53e3 100644
> > --- a/drivers/video/console/vgacon.c
> > +++ b/drivers/video/console/vgacon.c
> > @@ -1168,7 +1168,7 @@ static bool vgacon_scroll(struct vc_data *c, unsigned int t, unsigned int b,
> > c->vc_screenbuf_size - delta);
> > c->vc_origin = vga_vram_end - c->vc_screenbuf_size;
> > vga_rolled_over = 0;
> > - } else
> > + } else if (oldo - delta >= (unsigned long)c->vc_screenbuf)
> > c->vc_origin -= delta;
> > c->vc_scr_end = c->vc_origin + c->vc_screenbuf_size;
> > scr_memsetw((u16 *) (c->vc_origin), c->vc_video_erase_char,
>
>
> The patch is not wrong, but I'm not yet sure if it hides another issue.
> I've applied it to the fbdev tree for now (unless Greg wants to handle it).
Nope, you can handle it, thanks!
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH RFC 1/1] vgacon: Add check for vc_origin address range in vgacon_scroll()
From: Helge Deller @ 2025-04-27 15:34 UTC (permalink / raw)
To: GONG Ruiqi, Greg Kroah-Hartman, Jiri Slaby, linux-fbdev
Cc: security, Kees Cook, Yi Yang, Lu Jialin, Xiu Jianfeng
In-Reply-To: <20250427025303.888320-2-gongruiqi1@huawei.com>
On 4/27/25 04:53, GONG Ruiqi wrote:
> Our in-house Syzkaller reported the following BUG (twice), which we
> believed was the same issue with [1]:
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in vcs_scr_readw+0xc2/0xd0 drivers/tty/vt/vt.c:4740
> Read of size 2 at addr ffff88800f5bef60 by task syz.7.2620/12393
> ...
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0x72/0xa0 lib/dump_stack.c:106
> print_address_description.constprop.0+0x6b/0x3d0 mm/kasan/report.c:364
> print_report+0xba/0x280 mm/kasan/report.c:475
> kasan_report+0xa9/0xe0 mm/kasan/report.c:588
> vcs_scr_readw+0xc2/0xd0 drivers/tty/vt/vt.c:4740
> vcs_write_buf_noattr drivers/tty/vt/vc_screen.c:493 [inline]
> vcs_write+0x586/0x840 drivers/tty/vt/vc_screen.c:690
> vfs_write+0x219/0x960 fs/read_write.c:584
> ksys_write+0x12e/0x260 fs/read_write.c:639
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x59/0x110 arch/x86/entry/common.c:81
> entry_SYSCALL_64_after_hwframe+0x78/0xe2
> ...
> </TASK>
>
> Allocated by task 5614:
> kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
> kasan_set_track+0x25/0x30 mm/kasan/common.c:52
> ____kasan_kmalloc mm/kasan/common.c:374 [inline]
> __kasan_kmalloc+0x8f/0xa0 mm/kasan/common.c:383
> kasan_kmalloc include/linux/kasan.h:201 [inline]
> __do_kmalloc_node mm/slab_common.c:1007 [inline]
> __kmalloc+0x62/0x140 mm/slab_common.c:1020
> kmalloc include/linux/slab.h:604 [inline]
> kzalloc include/linux/slab.h:721 [inline]
> vc_do_resize+0x235/0xf40 drivers/tty/vt/vt.c:1193
> vgacon_adjust_height+0x2d4/0x350 drivers/video/console/vgacon.c:1007
> vgacon_font_set+0x1f7/0x240 drivers/video/console/vgacon.c:1031
> con_font_set drivers/tty/vt/vt.c:4628 [inline]
> con_font_op+0x4da/0xa20 drivers/tty/vt/vt.c:4675
> vt_k_ioctl+0xa10/0xb30 drivers/tty/vt/vt_ioctl.c:474
> vt_ioctl+0x14c/0x1870 drivers/tty/vt/vt_ioctl.c:752
> tty_ioctl+0x655/0x1510 drivers/tty/tty_io.c:2779
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:871 [inline]
> __se_sys_ioctl+0x12d/0x190 fs/ioctl.c:857
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x59/0x110 arch/x86/entry/common.c:81
> entry_SYSCALL_64_after_hwframe+0x78/0xe2
>
> Last potentially related work creation:
> kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
> __kasan_record_aux_stack+0x94/0xa0 mm/kasan/generic.c:492
> __call_rcu_common.constprop.0+0xc3/0xa10 kernel/rcu/tree.c:2713
> netlink_release+0x620/0xc20 net/netlink/af_netlink.c:802
> __sock_release+0xb5/0x270 net/socket.c:663
> sock_close+0x1e/0x30 net/socket.c:1425
> __fput+0x408/0xab0 fs/file_table.c:384
> __fput_sync+0x4c/0x60 fs/file_table.c:465
> __do_sys_close fs/open.c:1580 [inline]
> __se_sys_close+0x68/0xd0 fs/open.c:1565
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x59/0x110 arch/x86/entry/common.c:81
> entry_SYSCALL_64_after_hwframe+0x78/0xe2
>
> Second to last potentially related work creation:
> kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
> __kasan_record_aux_stack+0x94/0xa0 mm/kasan/generic.c:492
> __call_rcu_common.constprop.0+0xc3/0xa10 kernel/rcu/tree.c:2713
> netlink_release+0x620/0xc20 net/netlink/af_netlink.c:802
> __sock_release+0xb5/0x270 net/socket.c:663
> sock_close+0x1e/0x30 net/socket.c:1425
> __fput+0x408/0xab0 fs/file_table.c:384
> task_work_run+0x154/0x240 kernel/task_work.c:239
> exit_task_work include/linux/task_work.h:45 [inline]
> do_exit+0x8e5/0x1320 kernel/exit.c:874
> do_group_exit+0xcd/0x280 kernel/exit.c:1023
> get_signal+0x1675/0x1850 kernel/signal.c:2905
> arch_do_signal_or_restart+0x80/0x3b0 arch/x86/kernel/signal.c:310
> exit_to_user_mode_loop kernel/entry/common.c:111 [inline]
> exit_to_user_mode_prepare include/linux/entry-common.h:328 [inline]
> __syscall_exit_to_user_mode_work kernel/entry/common.c:207 [inline]
> syscall_exit_to_user_mode+0x1b3/0x1e0 kernel/entry/common.c:218
> do_syscall_64+0x66/0x110 arch/x86/entry/common.c:87
> entry_SYSCALL_64_after_hwframe+0x78/0xe2
>
> The buggy address belongs to the object at ffff88800f5be000
> which belongs to the cache kmalloc-2k of size 2048
> The buggy address is located 2656 bytes to the right of
> allocated 1280-byte region [ffff88800f5be000, ffff88800f5be500)
>
> ...
>
> Memory state around the buggy address:
> ffff88800f5bee00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff88800f5bee80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> ffff88800f5bef00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ^
> ffff88800f5bef80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff88800f5bf000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==================================================================
>
> By analyzing the vmcore, we found that vc->vc_origin was somehow placed
> one line prior to vc->vc_screenbuf when vc was in KD_TEXT mode, and
> further writings to /dev/vcs caused out-of-bounds reads (and writes
> right after) in vcs_write_buf_noattr().
>
> Our further experiments show that in most cases, vc->vc_origin equals to
> vga_vram_base when the console is in KD_TEXT mode, and it's around
> vc->vc_screenbuf for the KD_GRAPHICS mode. But via triggerring a
> TIOCL_SETVESABLANK ioctl beforehand, we can make vc->vc_origin be around
> vc->vc_screenbuf while the console is in KD_TEXT mode, and then by
> writing the special 'ESC M' control sequence to the tty certain times
> (depends on the value of `vc->state.y - vc->vc_top`), we can eventually
> move vc->vc_origin prior to vc->vc_screenbuf. Here's the PoC, tested on
> QEMU:
>
> ```
> int main() {
> const int RI_NUM = 10; // should be greater than `vc->state.y - vc->vc_top`
> int tty_fd, vcs_fd;
> const char *tty_path = "/dev/tty0";
> const char *vcs_path = "/dev/vcs";
> const char escape_seq[] = "\x1bM"; // ESC + M
> const char trigger_seq[] = "Let's trigger an OOB write.";
> struct vt_sizes vt_size = { 70, 2 };
> int blank = TIOCL_BLANKSCREEN;
>
> tty_fd = open(tty_path, O_RDWR);
>
> char vesa_mode[] = { TIOCL_SETVESABLANK, 1 };
> ioctl(tty_fd, TIOCLINUX, vesa_mode);
>
> ioctl(tty_fd, TIOCLINUX, &blank);
> ioctl(tty_fd, VT_RESIZE, &vt_size);
>
> for (int i = 0; i < RI_NUM; ++i)
> write(tty_fd, escape_seq, sizeof(escape_seq) - 1);
>
> vcs_fd = open(vcs_path, O_RDWR);
> write(vcs_fd, trigger_seq, sizeof(trigger_seq));
>
> close(vcs_fd);
> close(tty_fd);
> return 0;
> }
> ```
>
> To solve this problem, add an address range validation check in
> vgacon_scroll(), ensuring vc->vc_origin never precedes vc_screenbuf.
>
> Reported-by: syzbot+9c09fda97a1a65ea859b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9c09fda97a1a65ea859b [1]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Co-developed-by: Yi Yang <yiyang13@huawei.com>
> Signed-off-by: Yi Yang <yiyang13@huawei.com>
> Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>
> ---
> drivers/video/console/vgacon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index 37bd18730fe0..f9cdbf8c53e3 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -1168,7 +1168,7 @@ static bool vgacon_scroll(struct vc_data *c, unsigned int t, unsigned int b,
> c->vc_screenbuf_size - delta);
> c->vc_origin = vga_vram_end - c->vc_screenbuf_size;
> vga_rolled_over = 0;
> - } else
> + } else if (oldo - delta >= (unsigned long)c->vc_screenbuf)
> c->vc_origin -= delta;
> c->vc_scr_end = c->vc_origin + c->vc_screenbuf_size;
> scr_memsetw((u16 *) (c->vc_origin), c->vc_video_erase_char,
The patch is not wrong, but I'm not yet sure if it hides another issue.
I've applied it to the fbdev tree for now (unless Greg wants to handle it).
Thanks!
Helge
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox