Bug with /dev/vcs* devices

Bug with /dev/vcs* devices

[Storm Dragon]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

Howdy,

This is my first time ever submitting a kernel bug, so I apologize if this is the wrong place. As per the instructions I found, I first filed a bug in Arch Linux:

https://bugs.archlinux.org/task/77575

I will add the contents of the response here:

For the record, here are the 2 associated mailing list threads [1][2].
                                                                                                                                                                 
The reason why it affects both LTS and current linux is because it appears a security patch was applied to both trees to fix a UAF (use after free) bug.
                                                                                                                                                                 
If that patch is problematic, you should report it upstream to the kernel folks. Maybe even email the patch author directly. It's this patch [3]. Please let us
know what you find out.
                                                                                                                                                                 
[1] https://lists.archlinux.org/archives/list/arch-general%40lists.archlinux.org/thread/EOSHIVGUZLNAD7BPHSUGOXFYSAFWDYH7/
[2] https://lists.archlinux.org/archives/list/arch-general%40lists.archlinux.org/thread/S6R6UDUMX2LWL4HJ74MFNYOES45UBFLF/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/tty/vt/vc_screen.c?h=v5.15.94&id=fc9e27f3ba083534b8bbf72ab0f5c810ffdc7d18

----------End Response ----------

Methods for reproducing the bug are included in the link to the bug above. But I will include one of them here as well. As root:

[root@mjollnir ~]# cat /dev/vcsa1
                                                                                                                                                                 
    cat: /dev/vcsa1: No such device or address

The contents is displayed very briefly before the No such device or address error.

Thank you,
Storm

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Bug with /dev/vcs* devices

[Randy Dunlap]

[add George and Greg]

@Storm: You should always send emails directly to someone as well as to the mailing list.


On 2/19/23 13:21, Storm Dragon wrote:
> Howdy,
> 
> This is my first time ever submitting a kernel bug, so I apologize if this is the wrong place. As per the instructions I found, I first filed a bug in Arch Linux:
> 
> https://bugs.archlinux.org/task/77575
> 
> I will add the contents of the response here:
> 
> For the record, here are the 2 associated mailing list threads [1][2].
>                                                                                                                                                                 The reason why it affects both LTS and current linux is because it appears a security patch was applied to both trees to fix a UAF (use after free) bug.
>                                                                                                                                                                 If that patch is problematic, you should report it upstream to the kernel folks. Maybe even email the patch author directly. It's this patch [3]. Please let us
> know what you find out.
>                                                                                                                                                                 [1] https://lists.archlinux.org/archives/list/arch-general%40lists.archlinux.org/thread/EOSHIVGUZLNAD7BPHSUGOXFYSAFWDYH7/
> [2] https://lists.archlinux.org/archives/list/arch-general%40lists.archlinux.org/thread/S6R6UDUMX2LWL4HJ74MFNYOES45UBFLF/
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/tty/vt/vc_screen.c?h=v5.15.94&id=fc9e27f3ba083534b8bbf72ab0f5c810ffdc7d18
> 
> ----------End Response ----------
> 
> Methods for reproducing the bug are included in the link to the bug above. But I will include one of them here as well. As root:
> 
> [root@mjollnir ~]# cat /dev/vcsa1
>                                                                                                                                                                    cat: /dev/vcsa1: No such device or address
> 
> The contents is displayed very briefly before the No such device or address error.
> 
> Thank you,
> Storm

-- 
~Randy

Re: Bug with /dev/vcs* devices

[Storm Dragon]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

On Sun, Feb 19, 2023 at 02:11:00PM -0800, Randy Dunlap wrote:
>[add George and Greg]
>
>@Storm: You should always send emails directly to someone as well as to the mailing list.
>
>

Howdy Randy,

Thank you for adding the correct people. I thought I put someone in CC,
but apparently it did not work as expected. Part of the problem caused
by this bug is it causes my screen reader to behave a little strangely
sometimes. in this case, it read the CC field, but I must have put the
address in a different field, maybe bcc, or somewhere that caused it to
be dropped. Of course, it is probably a good thing, because the address
I found was not the one you added.

Thanks,
Storm


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] vc_screen: don't clobber return value in vcs_read

[linux]

From: Thomas Weißschuh <linux@weissschuh.net>

Commit 226fae124b2d
("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
moved the call to vcs_vc() into the loop.
While doing this it also moved the unconditional assignment of
"ret = -ENXIO".
This unconditional assignment was valid outside the loop but within it
it clobbers the actual value of ret.

To avoid this only assign "ret = -ENXIO" when actually needed.

Reported-by: Storm Dragon <stormdragon2976@gmail.com>
Link: https://lore.kernel.org/lkml/Y%2FKS6vdql2pIsCiI@hotmail.com/
Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

---

@Storm Could you validate this patch?
---
 drivers/tty/vt/vc_screen.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index f566eb1839dc..2ef519a40a87 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		unsigned int this_round, skip = 0;
 		int size;
 
-		ret = -ENXIO;
 		vc = vcs_vc(inode, &viewed);
-		if (!vc)
+		if (!vc) {
+			ret = -ENXIO;
 			goto unlock_out;
+		}
 
 		/* Check whether we are above size each round,
 		 * as copy_to_user at the end of this loop

base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
-- 
2.39.2

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Jiri Slaby]

On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
> From: Thomas Weißschuh <linux@weissschuh.net>
> 
> Commit 226fae124b2d
> ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> moved the call to vcs_vc() into the loop.
> While doing this it also moved the unconditional assignment of
> "ret = -ENXIO".
> This unconditional assignment was valid outside the loop but within it
> it clobbers the actual value of ret.
> 
> To avoid this only assign "ret = -ENXIO" when actually needed.

Not sure -- I cannot find it -- but hasn't George fixed this yet?

> Reported-by: Storm Dragon <stormdragon2976@gmail.com>
> Link: https://lore.kernel.org/lkml/Y%2FKS6vdql2pIsCiI@hotmail.com/
> Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> ---
> 
> @Storm Could you validate this patch?
> ---
>   drivers/tty/vt/vc_screen.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> index f566eb1839dc..2ef519a40a87 100644
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   		unsigned int this_round, skip = 0;
>   		int size;
>   
> -		ret = -ENXIO;
>   		vc = vcs_vc(inode, &viewed);
> -		if (!vc)
> +		if (!vc) {
> +			ret = -ENXIO;
>   			goto unlock_out;
> +		}
>   
>   		/* Check whether we are above size each round,
>   		 * as copy_to_user at the end of this loop
> 
> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c

-- 
js
suse labs

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Thomas Weißschuh]

+Cc people who were involved in the original thread.

On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
> On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
> > From: Thomas Weißschuh <linux@weissschuh.net>
> > 
> > Commit 226fae124b2d
> > ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> > moved the call to vcs_vc() into the loop.
> > While doing this it also moved the unconditional assignment of
> > "ret = -ENXIO".
> > This unconditional assignment was valid outside the loop but within it
> > it clobbers the actual value of ret.
> > 
> > To avoid this only assign "ret = -ENXIO" when actually needed.
> 
> Not sure -- I cannot find it -- but hasn't George fixed this yet?

Indeed there was a proposed fix at
https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/

Linus had some suggestions so it was not applied as is.

I'm not sure what the current state is.
George, do you have something in the pipeline?

I also tested the patch proposed by Linus as attachment and that works.
(The small inline patch snippet doesn't)

> > Reported-by: Storm Dragon <stormdragon2976@gmail.com>
> > Link: https://lore.kernel.org/lkml/Y%2FKS6vdql2pIsCiI@hotmail.com/
> > Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > 
> > ---
> > 
> > @Storm Could you validate this patch?
> > ---
> >   drivers/tty/vt/vc_screen.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> > index f566eb1839dc..2ef519a40a87 100644
> > --- a/drivers/tty/vt/vc_screen.c
> > +++ b/drivers/tty/vt/vc_screen.c
> > @@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> >   		unsigned int this_round, skip = 0;
> >   		int size;
> > -		ret = -ENXIO;
> >   		vc = vcs_vc(inode, &viewed);
> > -		if (!vc)
> > +		if (!vc) {
> > +			ret = -ENXIO;
> >   			goto unlock_out;
> > +		}
> >   		/* Check whether we are above size each round,
> >   		 * as copy_to_user at the end of this loop
> > 
> > base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
> 
> -- 
> js
> suse labs
> 

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[George Kennedy]

On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
> +Cc people who were involved in the original thread.
>
> On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
>> On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
>>> From: Thomas Weißschuh <linux@weissschuh.net>
>>>
>>> Commit 226fae124b2d
>>> ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
>>> moved the call to vcs_vc() into the loop.
>>> While doing this it also moved the unconditional assignment of
>>> "ret = -ENXIO".
>>> This unconditional assignment was valid outside the loop but within it
>>> it clobbers the actual value of ret.
>>>
>>> To avoid this only assign "ret = -ENXIO" when actually needed.
>> Not sure -- I cannot find it -- but hasn't George fixed this yet?
> Indeed there was a proposed fix at
> https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/
>
> Linus had some suggestions so it was not applied as is.
>
> I'm not sure what the current state is.
> George, do you have something in the pipeline?

Yes, that is in the pipeline:
https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/

Linus suggested the fix, which was tested and submitted.

Jiri commented on the patch, which I believe was directed at Linus as he 
suggested the fix.

George
>
> I also tested the patch proposed by Linus as attachment and that works.
> (The small inline patch snippet doesn't)
>
>>> Reported-by: Storm Dragon <stormdragon2976@gmail.com>
>>> Link: https://lore.kernel.org/lkml/Y%2FKS6vdql2pIsCiI@hotmail.com/
>>> Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>
>>> ---
>>>
>>> @Storm Could you validate this patch?
>>> ---
>>>    drivers/tty/vt/vc_screen.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
>>> index f566eb1839dc..2ef519a40a87 100644
>>> --- a/drivers/tty/vt/vc_screen.c
>>> +++ b/drivers/tty/vt/vc_screen.c
>>> @@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>>    		unsigned int this_round, skip = 0;
>>>    		int size;
>>> -		ret = -ENXIO;
>>>    		vc = vcs_vc(inode, &viewed);
>>> -		if (!vc)
>>> +		if (!vc) {
>>> +			ret = -ENXIO;
>>>    			goto unlock_out;
>>> +		}
>>>    		/* Check whether we are above size each round,
>>>    		 * as copy_to_user at the end of this loop
>>>
>>> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
>> -- 
>> js
>> suse labs
>>

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Thomas Weißschuh]

On Tue, Feb 21, 2023 at 08:30:11AM -0500, George Kennedy wrote:
> 
> 
> On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
> > +Cc people who were involved in the original thread.
> > 
> > On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
> > > On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
> > > > From: Thomas Weißschuh <linux@weissschuh.net>
> > > > 
> > > > Commit 226fae124b2d
> > > > ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> > > > moved the call to vcs_vc() into the loop.
> > > > While doing this it also moved the unconditional assignment of
> > > > "ret = -ENXIO".
> > > > This unconditional assignment was valid outside the loop but within it
> > > > it clobbers the actual value of ret.
> > > > 
> > > > To avoid this only assign "ret = -ENXIO" when actually needed.
> > > Not sure -- I cannot find it -- but hasn't George fixed this yet?
> > Indeed there was a proposed fix at
> > https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/
> > 
> > Linus had some suggestions so it was not applied as is.
> > 
> > I'm not sure what the current state is.
> > George, do you have something in the pipeline?
> 
> Yes, that is in the pipeline:
> https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
> 
> Linus suggested the fix, which was tested and submitted.
> 
> Jiri commented on the patch, which I believe was directed at Linus as he
> suggested the fix.

Thanks for the pointer!

I searched for it by its Fixes: tag.
The v2 has a different one than the v1.
To me the v1 Fixes: seems more correct, was the change
intentional?

> George
> > 
> > I also tested the patch proposed by Linus as attachment and that works.
> > (The small inline patch snippet doesn't)
> > 
> > > > Reported-by: Storm Dragon <stormdragon2976@gmail.com>
> > > > Link: https://lore.kernel.org/lkml/Y%2FKS6vdql2pIsCiI@hotmail.com/
> > > > Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > 
> > > > ---
> > > > 
> > > > @Storm Could you validate this patch?
> > > > ---
> > > >    drivers/tty/vt/vc_screen.c | 5 +++--
> > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> > > > index f566eb1839dc..2ef519a40a87 100644
> > > > --- a/drivers/tty/vt/vc_screen.c
> > > > +++ b/drivers/tty/vt/vc_screen.c
> > > > @@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> > > >    		unsigned int this_round, skip = 0;
> > > >    		int size;
> > > > -		ret = -ENXIO;
> > > >    		vc = vcs_vc(inode, &viewed);
> > > > -		if (!vc)
> > > > +		if (!vc) {
> > > > +			ret = -ENXIO;
> > > >    			goto unlock_out;
> > > > +		}
> > > >    		/* Check whether we are above size each round,
> > > >    		 * as copy_to_user at the end of this loop
> > > > 
> > > > base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
> > > -- 
> > > js
> > > suse labs
> > > 
> 

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Greg Kroah-Hartman]

On Tue, Feb 21, 2023 at 08:30:11AM -0500, George Kennedy wrote:
> 
> 
> On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
> > +Cc people who were involved in the original thread.
> > 
> > On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
> > > On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
> > > > From: Thomas Weißschuh <linux@weissschuh.net>
> > > > 
> > > > Commit 226fae124b2d
> > > > ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
> > > > moved the call to vcs_vc() into the loop.
> > > > While doing this it also moved the unconditional assignment of
> > > > "ret = -ENXIO".
> > > > This unconditional assignment was valid outside the loop but within it
> > > > it clobbers the actual value of ret.
> > > > 
> > > > To avoid this only assign "ret = -ENXIO" when actually needed.
> > > Not sure -- I cannot find it -- but hasn't George fixed this yet?
> > Indeed there was a proposed fix at
> > https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/
> > 
> > Linus had some suggestions so it was not applied as is.
> > 
> > I'm not sure what the current state is.
> > George, do you have something in the pipeline?
> 
> Yes, that is in the pipeline:
> https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
> 
> Linus suggested the fix, which was tested and submitted.
> 
> Jiri commented on the patch, which I believe was directed at Linus as he
> suggested the fix.

And I was waiting for a new version from you based on those comments :(

Can you fix that up and send?

thanks,

greg k-h

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Linux regression tracking (Thorsten Leemhuis)]

Hi, this is your Linux kernel regression tracker. Top-posting for once,
to make this easily accessible to everyone.

George, is there anything we can do to help you moving forward to
finally get this regression fixed? It seems (or am I missing something?)
everyone is waiting for you (see below) to act on the feedback Jiri
provided here:

https://lore.kernel.org/lkml/8dffe187-240d-746e-ed84-885ffd2785f6@kernel.org/

Side note: would be good to add a "Link:" tag pointing to the start of
this thread as well, but that's just a detail.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

On 21.02.23 14:50, Greg Kroah-Hartman wrote:
> On Tue, Feb 21, 2023 at 08:30:11AM -0500, George Kennedy wrote:
>> On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
>>> +Cc people who were involved in the original thread.
>>>
>>> On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
>>>> On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
>>>>> From: Thomas Weißschuh <linux@weissschuh.net>
>>>>>
>>>>> Commit 226fae124b2d
>>>>> ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
>>>>> moved the call to vcs_vc() into the loop.
>>>>> While doing this it also moved the unconditional assignment of
>>>>> "ret = -ENXIO".
>>>>> This unconditional assignment was valid outside the loop but within it
>>>>> it clobbers the actual value of ret.
>>>>>
>>>>> To avoid this only assign "ret = -ENXIO" when actually needed.
>>>> Not sure -- I cannot find it -- but hasn't George fixed this yet?
>>> Indeed there was a proposed fix at
>>> https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/
>>>
>>> Linus had some suggestions so it was not applied as is.
>>>
>>> I'm not sure what the current state is.
>>> George, do you have something in the pipeline?
>>
>> Yes, that is in the pipeline:
>> https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
>>
>> Linus suggested the fix, which was tested and submitted.
>>
>> Jiri commented on the patch, which I believe was directed at Linus as he
>> suggested the fix.
> 
> And I was waiting for a new version from you based on those comments :(
> 
> Can you fix that up and send?
> 
> thanks,
> 
> greg k-h

#regzbot monitor:
https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
#regzbot poke

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[George Kennedy]

Hello Thomas,

On 2/27/2023 9:20 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
> Hi, this is your Linux kernel regression tracker. Top-posting for once,
> to make this easily accessible to everyone.
>
> George, is there anything we can do to help you moving forward to
> finally get this regression fixed? It seems (or am I missing something?)
> everyone is waiting for you (see below) to act on the feedback Jiri
> provided here:
>
> https://lore.kernel.org/lkml/8dffe187-240d-746e-ed84-885ffd2785f6@kernel.org/
>
> Side note: would be good to add a "Link:" tag pointing to the start of
> this thread as well, but that's just a detail.

I just sent the requested patch up for review.

https://lore.kernel.org/lkml/1677527001-17459-1-git-send-email-george.kennedy@oracle.com/

Last post on the previous patch that led to the requested patch:
https://lore.kernel.org/lkml/9e297f30-dc8c-ecac-f7a6-348ddbd4b928@leemhuis.info/

Thank you,
George
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
> On 21.02.23 14:50, Greg Kroah-Hartman wrote:
>> On Tue, Feb 21, 2023 at 08:30:11AM -0500, George Kennedy wrote:
>>> On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
>>>> +Cc people who were involved in the original thread.
>>>>
>>>> On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
>>>>> On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
>>>>>> From: Thomas Weißschuh <linux@weissschuh.net>
>>>>>>
>>>>>> Commit 226fae124b2d
>>>>>> ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
>>>>>> moved the call to vcs_vc() into the loop.
>>>>>> While doing this it also moved the unconditional assignment of
>>>>>> "ret = -ENXIO".
>>>>>> This unconditional assignment was valid outside the loop but within it
>>>>>> it clobbers the actual value of ret.
>>>>>>
>>>>>> To avoid this only assign "ret = -ENXIO" when actually needed.
>>>>> Not sure -- I cannot find it -- but hasn't George fixed this yet?
>>>> Indeed there was a proposed fix at
>>>> https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/
>>>>
>>>> Linus had some suggestions so it was not applied as is.
>>>>
>>>> I'm not sure what the current state is.
>>>> George, do you have something in the pipeline?
>>> Yes, that is in the pipeline:
>>> https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
>>>
>>> Linus suggested the fix, which was tested and submitted.
>>>
>>> Jiri commented on the patch, which I believe was directed at Linus as he
>>> suggested the fix.
>> And I was waiting for a new version from you based on those comments :(
>>
>> Can you fix that up and send?
>>
>> thanks,
>>
>> greg k-h
> #regzbot monitor:
> https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
> #regzbot poke

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Linux regression tracking (Thorsten Leemhuis)]

On 27.02.23 20:59, George Kennedy wrote:
> Hello Thomas,
> 
> On 2/27/2023 9:20 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
>> Hi, this is your Linux kernel regression tracker. Top-posting for once,
>> to make this easily accessible to everyone.
>>
>> George, is there anything we can do to help you moving forward to
>> finally get this regression fixed? It seems (or am I missing something?)
>> everyone is waiting for you (see below) to act on the feedback Jiri
>> provided here:
>>
>> https://lore.kernel.org/lkml/8dffe187-240d-746e-ed84-885ffd2785f6@kernel.org/
>>
>> Side note: would be good to add a "Link:" tag pointing to the start of
>> this thread as well, but that's just a detail.
> 
> I just sent the requested patch up for review.
> 
> https://lore.kernel.org/lkml/1677527001-17459-1-git-send-email-george.kennedy@oracle.com/

Thx for handling this!

And I see it very quickly made its way to mainline. :-D

Ciao, Thorsten

>> On 21.02.23 14:50, Greg Kroah-Hartman wrote:
>>> On Tue, Feb 21, 2023 at 08:30:11AM -0500, George Kennedy wrote:
>>>> On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
>>>>> +Cc people who were involved in the original thread.
>>>>>
>>>>> On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
>>>>>> On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
>>>>>>> From: Thomas Weißschuh <linux@weissschuh.net>
>>>>>>>
>>>>>>> Commit 226fae124b2d
>>>>>>> ("vc_screen: move load of struct vc_data pointer in vcs_read() to
>>>>>>> avoid UAF")
>>>>>>> moved the call to vcs_vc() into the loop.
>>>>>>> While doing this it also moved the unconditional assignment of
>>>>>>> "ret = -ENXIO".
>>>>>>> This unconditional assignment was valid outside the loop but
>>>>>>> within it
>>>>>>> it clobbers the actual value of ret.
>>>>>>>
>>>>>>> To avoid this only assign "ret = -ENXIO" when actually needed.
>>>>>> Not sure -- I cannot find it -- but hasn't George fixed this yet?
>>>>> Indeed there was a proposed fix at
>>>>> https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/
>>>>>
>>>>> Linus had some suggestions so it was not applied as is.
>>>>>
>>>>> I'm not sure what the current state is.
>>>>> George, do you have something in the pipeline?
>>>> Yes, that is in the pipeline:
>>>> https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
>>>>
>>>> Linus suggested the fix, which was tested and submitted.
>>>>
>>>> Jiri commented on the patch, which I believe was directed at Linus
>>>> as he
>>>> suggested the fix.
>>> And I was waiting for a new version from you based on those comments :(
>>>
>>> Can you fix that up and send?
>>>
>>> thanks,
>>>
>>> greg k-h
>> #regzbot monitor:
>> https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/
>> #regzbot poke
> 
> 
> 

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Storm Dragon]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

On Mon, Feb 20, 2023 at 06:46:12AM +0000, linux@weissschuh.net wrote:
>From: Thomas Weißschuh <linux@weissschuh.net>
>
>@Storm Could you validate this patch?


I am willing to test, but I have almost no experience doing anything
with the kernel other than upgrading it from time to time. Can you send
me some instructions for testing this?

Thanks,
Storm

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Thomas Weißschuh]

On Mon, Feb 20, 2023 at 11:06:08AM -0500, Storm Dragon wrote:
> On Mon, Feb 20, 2023 at 06:46:12AM +0000, linux@weissschuh.net wrote:
> > From: Thomas Weißschuh <linux@weissschuh.net>
> > 
> > @Storm Could you validate this patch?
> 
> 
> I am willing to test, but I have almost no experience doing anything
> with the kernel other than upgrading it from time to time. Can you send
> me some instructions for testing this?

Sorry for the long delay, but this should be fixed in the current round
of stable kernels. Can you try the following:

pacman -U https://mirrors.edge.kernel.org/archlinux/testing/os/x86_64/linux-6.2.2.arch1-1-x86_64.pkg.tar.zst

Thomas

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Storm Dragon]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Fri, Mar 03, 2023 at 09:12:50PM +0000, Thomas Weißschuh wrote:
>Sorry for the long delay, but this should be fixed in the current round
>of stable kernels. Can you try the following:
>
>pacman -U https://mirrors.edge.kernel.org/archlinux/testing/os/x86_64/linux-6.2.2.arch1-1-x86_64.pkg.tar.zst
>
>Thomas

I have installed the package above. My screen reader is behaving much
better now. Interestingly, however, trying to cat the /dev/vcs device
still shows the following:

cat: /dev/vcs: No such device or address

cat: /dev/vcsa: No such device or address

cat: /dev/vcsa1: No such device or address

Is this expected behavior? 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Thomas Weißschuh]

On Fri, Mar 03, 2023 at 04:46:21PM -0500, Storm Dragon wrote:
> On Fri, Mar 03, 2023 at 09:12:50PM +0000, Thomas Weißschuh wrote:
> > Sorry for the long delay, but this should be fixed in the current round
> > of stable kernels. Can you try the following:
> > 
> > pacman -U https://mirrors.edge.kernel.org/archlinux/testing/os/x86_64/linux-6.2.2.arch1-1-x86_64.pkg.tar.zst
> > 
> > Thomas
> 
> I have installed the package above. My screen reader is behaving much
> better now. Interestingly, however, trying to cat the /dev/vcs device
> still shows the following:
>
> cat: /dev/vcs: No such device or address
> 
> cat: /dev/vcsa: No such device or address
> 
> cat: /dev/vcsa1: No such device or address
> 
> Is this expected behavior?

No it isn't. 

Is this reliably reproducible? I doesn't happen on my side.
Maybe you can provide more detailed reproduction steps.

Just to be sure; did you reboot into the new kernel?

Does this mean the screenreader now works correctly or is it still
broken somehow?

Thomas

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Storm Dragon]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On Fri, Mar 03, 2023 at 11:25:00PM +0000, Thomas Weißschuh wrote:
>No it isn't.
>
>Is this reliably reproducible? I doesn't happen on my side.
>Maybe you can provide more detailed reproduction steps.
>
>Just to be sure; did you reboot into the new kernel?
>
>Does this mean the screenreader now works correctly or is it still
>broken somehow?
>
>Thomas

I found the problem when I was writing the steps I did to reproduce it.
I did install the kernel, I did reboot, but I did not uninstall the lts
kernel I was using currently, nor did I run grub-mkconfig. Now that I
have done things the right way, it works as expected. Everything is
awesome again.

Thanks, and sorry for the false alarm.
Storm

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Storm Dragon]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

On Fri, Mar 03, 2023 at 11:25:00PM +0000, Thomas Weißschuh wrote:
>Does this mean the screenreader now works correctly or is it still
>broken somehow?
>
>Thomas

I forgot to answer the screen reader question. I hadn't rebooted in a
while, and I forgot that the work around Chrys put in place for Fenrir
does really well until the screen fills up. So, right after I sent the
message saying it was working better, it reverted to doing its weird
jumps and things caused by the bug. Now that the correct version is
actually running, it really is better.

Thanks,
Storm


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Storm Dragon]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

On Fri, Mar 03, 2023 at 11:25:00PM +0000, Thomas Weißschuh wrote:

>Does this mean the screenreader now works correctly or is it still
>broken somehow?
>
>Thomas

I have still been testing this kernel. Most things work as expected, but
the pasting functionality for Fenrir's clipboard is broken. After
checking into the problem, it seems that tiocsti is disabled, and that
is causing the problem. Was that something done in this test kernel
only, or will that be the default for all new Arch kernels? If it is the
default, is there a way to turn it back on? I tried the following:

[storm@mjollnir ~] $ sudo sysctl dev.tty.legacy_tiocsti=1
sysctl: setting key "dev.tty.legacy_tiocsti": Invalid argument

Thanks,
Storm

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Thomas Weißschuh]

On Sat, Mar 04, 2023 at 02:58:46PM -0500, Storm Dragon wrote:
> On Fri, Mar 03, 2023 at 11:25:00PM +0000, Thomas Weißschuh wrote:
> 
> > Does this mean the screenreader now works correctly or is it still
> > broken somehow?
> > 
> > Thomas
> 
> I have still been testing this kernel. Most things work as expected, but
> the pasting functionality for Fenrir's clipboard is broken. After
> checking into the problem, it seems that tiocsti is disabled, and that
> is causing the problem. Was that something done in this test kernel
> only, or will that be the default for all new Arch kernels? If it is the
> default, is there a way to turn it back on?

By now this package has been promoted to [core] so it is in fact the
default ArchLinux kernel.

As a workaround you can use the "linux-lts" package that now also
carries the fix for your original problem but not the code responsible
for the new issue.

pacman -Sy linux-lts

> I tried the following:
> 
> [storm@mjollnir ~] $ sudo sysctl dev.tty.legacy_tiocsti=1
> sysctl: setting key "dev.tty.legacy_tiocsti": Invalid argument

This is indeed the correct way to enable the feature again.

It seems that the commit that introduced this sysctl[0] depends on
another commit [1] to be applied. But the 6.2.2 stable kernel is missing
the requirement.

I'll validate that this indeed is the issue and will then send a formal
request for backporting.

[0] 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled")
[1] f1aa2eb5ea05 ("sysctl: fix proc_dobool() usability")

Re: [PATCH] vc_screen: don't clobber return value in vcs_read

[Thomas Weißschuh]

On Sat, Mar 04, 2023 at 02:58:46PM -0500, Storm Dragon wrote:
> On Fri, Mar 03, 2023 at 11:25:00PM +0000, Thomas Weißschuh wrote:
> 
> > Does this mean the screenreader now works correctly or is it still
> > broken somehow?
> > 
> > Thomas
> 
> I have still been testing this kernel. Most things work as expected, but
> the pasting functionality for Fenrir's clipboard is broken. After
> checking into the problem, it seems that tiocsti is disabled, and that
> is causing the problem. Was that something done in this test kernel
> only, or will that be the default for all new Arch kernels? If it is the
> default, is there a way to turn it back on? I tried the following:
> 
> [storm@mjollnir ~] $ sudo sysctl dev.tty.legacy_tiocsti=1
> sysctl: setting key "dev.tty.legacy_tiocsti": Invalid argument

The sysctl to enable the ioctl should be fixed in 6.2.3:

pacman -U https://mirrors.edge.kernel.org/archlinux/testing/os/x86_64/linux-6.2.3.arch1-1-x86_64.pkg.tar.zst

Thomas