Text mode VGA-console scrolling is broken in upstream & stable trees

Text mode VGA-console scrolling is broken in upstream & stable trees

[Jari Ruusu]

The patch that broke text mode VGA-console scrolling is this one:
"vgacon: Add check for vc_origin address range in vgacon_scroll()"
commit 864f9963ec6b4b76d104d595ba28110b87158003 upstream.

How to preproduce:
(1) boot a kernel that is configured to use text mode VGA-console
(2) type commands:  ls -l /usr/bin | less -S
(3) scroll up/down with cursor-down/up keys

Above mentioned patch seems to have landed in upstream and all
kernel.org stable trees with zero testing. Even minimal testing
would have shown that it breaks text mode VGA-console scrolling.

Greg, Sasha, Linus,
Please consider reverting that buggy patch from all affected trees.

--
Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189

Re: Text mode VGA-console scrolling is broken in upstream & stable trees

[Greg Kroah-Hartman]

On Wed, Jul 30, 2025 at 02:06:27PM +0000, Jari Ruusu wrote:
> The patch that broke text mode VGA-console scrolling is this one:
> "vgacon: Add check for vc_origin address range in vgacon_scroll()"
> commit 864f9963ec6b4b76d104d595ba28110b87158003 upstream.
> 
> How to preproduce:
> (1) boot a kernel that is configured to use text mode VGA-console
> (2) type commands:  ls -l /usr/bin | less -S
> (3) scroll up/down with cursor-down/up keys
> 
> Above mentioned patch seems to have landed in upstream and all
> kernel.org stable trees with zero testing. Even minimal testing
> would have shown that it breaks text mode VGA-console scrolling.
> 
> Greg, Sasha, Linus,
> Please consider reverting that buggy patch from all affected trees.

Please work to fix it in Linus's tree first and then we will be glad to
backport the needed fix.

thanks,

greg k-h

Re: Text mode VGA-console scrolling is broken in upstream & stable trees

[Salvatore Bonaccorso]

Hi,

On Wed, Jul 30, 2025 at 04:26:44PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 30, 2025 at 02:06:27PM +0000, Jari Ruusu wrote:
> > The patch that broke text mode VGA-console scrolling is this one:
> > "vgacon: Add check for vc_origin address range in vgacon_scroll()"
> > commit 864f9963ec6b4b76d104d595ba28110b87158003 upstream.
> > 
> > How to preproduce:
> > (1) boot a kernel that is configured to use text mode VGA-console
> > (2) type commands:  ls -l /usr/bin | less -S
> > (3) scroll up/down with cursor-down/up keys
> > 
> > Above mentioned patch seems to have landed in upstream and all
> > kernel.org stable trees with zero testing. Even minimal testing
> > would have shown that it breaks text mode VGA-console scrolling.
> > 
> > Greg, Sasha, Linus,
> > Please consider reverting that buggy patch from all affected trees.
> 
> Please work to fix it in Linus's tree first and then we will be glad to
> backport the needed fix.

FWIW, and maybe just an interesting side node: if it ever get
considered to revert the commit, this will re-introduce/re-open
CVE-2025-38213.

Cf. https://lore.kernel.org/linux-cve-announce/2025070422-CVE-2025-38213-c3e3@gregkh/T/#u

Regards,
Salvatore

Re: Text mode VGA-console scrolling is broken in upstream & stable trees

[Jiri Slaby]

On 30. 07. 25, 16:06, Jari Ruusu wrote:
> The patch that broke text mode VGA-console scrolling is this one:
> "vgacon: Add check for vc_origin address range in vgacon_scroll()"
> commit 864f9963ec6b4b76d104d595ba28110b87158003 upstream.
> 
> How to preproduce:
> (1) boot a kernel that is configured to use text mode VGA-console
> (2) type commands:  ls -l /usr/bin | less -S
> (3) scroll up/down with cursor-down/up keys
> 
> Above mentioned patch seems to have landed in upstream and all
> kernel.org stable trees with zero testing. Even minimal testing
> would have shown that it breaks text mode VGA-console scrolling.

At the time this was posted (privately and on security@), I commented:

=====

 > --- 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;

IMO you should also add:
    else
      c->vc_origin = c->vc_screenbuf;

Or clamp 'delta' beforehand and don't add the 'if'.

=====

That did not happen, AFAICS. Care to test the above suggestion?

thanks,
-- 
js
suse labs

Re: Text mode VGA-console scrolling is broken in upstream & stable trees

[Jari Ruusu]

On Thursday, July 31st, 2025 at 10:22, Jiri Slaby <jirislaby@kernel.org> wrote:
> At the time this was posted (privately and on security@), I commented:
> =====
>  > --- 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;
> 
> IMO you should also add:
>     else
>       c->vc_origin = c->vc_screenbuf;
> 
> Or clamp 'delta' beforehand and don't add the 'if'.
> =====
> That did not happen, AFAICS. Care to test the above suggestion?

My reading of the code in vgacon_scroll() is that it directly
bit-bangs video-RAM and checks that scroll read/write accesses
stay in range vga_vram_base...vga_vram_end-1.

Checking that c->vc_origin end up being >= c->vc_screenbuf is
wrong because in text mode it should be index to video-RAM.

Quote from original "messed up" patch, fix for CVE-2025-38213:
> 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:

To me that sounds like the bug is in TIOCL_SETVESABLANK ioctl().
It should not be changing c->vc_origin to point elsewhere
other than video-RAM when the console is in text mode.

How about adding a check to begining of vgacon_scroll() that
bails out early if c->vc_origin is not a valid index to video-RAM?

--
Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189

Re: Text mode VGA-console scrolling is broken in upstream & stable trees

[Helge Deller]

On 7/31/25 18:12, Jari Ruusu wrote:
> On Thursday, July 31st, 2025 at 10:22, Jiri Slaby <jirislaby@kernel.org> wrote:
>> At the time this was posted (privately and on security@), I commented:
>> =====
>>   > --- 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;
>>
>> IMO you should also add:
>>      else
>>        c->vc_origin = c->vc_screenbuf;
>>
>> Or clamp 'delta' beforehand and don't add the 'if'.
>> =====
>> That did not happen, AFAICS. Care to test the above suggestion?
> 
> My reading of the code in vgacon_scroll() is that it directly
> bit-bangs video-RAM and checks that scroll read/write accesses
> stay in range vga_vram_base...vga_vram_end-1.
> 
> Checking that c->vc_origin end up being >= c->vc_screenbuf is
> wrong because in text mode it should be index to video-RAM.

Yes, correct.

  
> Quote from original "messed up" patch, fix for CVE-2025-38213:
>> 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:
> 
> To me that sounds like the bug is in TIOCL_SETVESABLANK ioctl().
> It should not be changing c->vc_origin to point elsewhere
> other than video-RAM when the console is in text mode.

Yes, agreed.
  
> How about adding a check to begining of vgacon_scroll() that
> bails out early if c->vc_origin is not a valid index to video-RAM?

Possible, but that's more of a work-around.
It would be nice to find and fix the real problem.

I tried to reproduce the issue with the provided testcase from
the original patch, but so far I failed.

For now I've added a Revert of the original patch to the fbdev git tree,
so that VGA backward scrolling works again. This gives some time to
fix the KASAN report.

Helge