tty: vt: selection: Add check for valid tiocl_selection values

[PATCH] tty: vt: selection: Add check for valid tiocl_selection values

Posted by Helge Deller 3 years, 8 months ago

The line and column numbers for the selection need to start at 1.
Add the checks to prevent invalid input.

Signed-off-by: Helge Deller <deller@gmx.de>
Reported-by: syzbot+14b0e8f3fd1612e35350@syzkaller.appspotmail.com

diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index f7755e73696e..58692a9b4097 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
 		return 0;
 	}

+	if (!v->xs || !v->ys || !v->xe || !v->ye)
+		return -EINVAL;
+
 	v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
 	v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
 	v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);

Re: [PATCH] tty: vt: selection: Add check for valid tiocl_selection values

Posted by Jiri Slaby 3 years, 8 months ago

On 30. 07. 22, 20:49, Helge Deller wrote:
> The line and column numbers for the selection need to start at 1.
> Add the checks to prevent invalid input.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Reported-by: syzbot+14b0e8f3fd1612e35350@syzkaller.appspotmail.com
> 
> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> index f7755e73696e..58692a9b4097 100644
> --- a/drivers/tty/vt/selection.c
> +++ b/drivers/tty/vt/selection.c
> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>   		return 0;
>   	}
> 
> +	if (!v->xs || !v->ys || !v->xe || !v->ye)
> +		return -EINVAL;

Hmm, I'm not sure about this. It potentially breaks userspace (by 
returning EINVAL now). And the code below should handle this just fine, 
right:

> +
>   	v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
>   	v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
>   	v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);

?

thanks,
-- 
js
suse labs

Re: [PATCH] tty: vt: selection: Add check for valid tiocl_selection values

Posted by Helge Deller 3 years, 8 months ago

Hello Jiri,

Thanks for looking into this patch!

On 8/4/22 07:47, Jiri Slaby wrote:
> On 30. 07. 22, 20:49, Helge Deller wrote:
>> The line and column numbers for the selection need to start at 1.
>> Add the checks to prevent invalid input.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Reported-by: syzbot+14b0e8f3fd1612e35350@syzkaller.appspotmail.com
>>
>> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
>> index f7755e73696e..58692a9b4097 100644
>> --- a/drivers/tty/vt/selection.c
>> +++ b/drivers/tty/vt/selection.c
>> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>>           return 0;
>>       }
>>
>> +    if (!v->xs || !v->ys || !v->xe || !v->ye)
>> +        return -EINVAL;
>
> Hmm, I'm not sure about this. It potentially breaks userspace (by
> returning EINVAL now).

Right.
According to the code below, my interpretation is that all xs/ys/xe/ye values
should be > 0. But of course I might be wrong on this, as I didn't find any
documentation for TIOCL_SETSEL.

And if userspace tries to set an invalid selection (e.g. by selecting row 0),
my patch now returns -EINVAL, while it returned success before.

> And the code below should handle this just fine, right:
>>       v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
>>       v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
>>       v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);

It "handles it fine" in the sense that it can cope with the
input and will not crash.
But it returns (maybe?) unexpected results...

For example, if a user selects row 0 (where I assume he wanted to set
the first line), he instead selects the last row.
I'm not sure if this is the expected behaviour.

Do you know of any userspace program which breaks because of this?

Helge

Re: [PATCH] tty: vt: selection: Add check for valid tiocl_selection values

Posted by Helge Deller 3 years, 8 months ago

On 8/4/22 09:15, Helge Deller wrote:
> Hello Jiri,
>
> Thanks for looking into this patch!
>
> On 8/4/22 07:47, Jiri Slaby wrote:
>> On 30. 07. 22, 20:49, Helge Deller wrote:
>>> The line and column numbers for the selection need to start at 1.
>>> Add the checks to prevent invalid input.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>> Reported-by: syzbot+14b0e8f3fd1612e35350@syzkaller.appspotmail.com
>>>
>>> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
>>> index f7755e73696e..58692a9b4097 100644
>>> --- a/drivers/tty/vt/selection.c
>>> +++ b/drivers/tty/vt/selection.c
>>> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>>>           return 0;
>>>       }
>>>
>>> +    if (!v->xs || !v->ys || !v->xe || !v->ye)
>>> +        return -EINVAL;
>>
>> Hmm, I'm not sure about this. It potentially breaks userspace (by
>> returning EINVAL now).
>
> Right.
> According to the code below, my interpretation is that all xs/ys/xe/ye values
> should be > 0. But of course I might be wrong on this, as I didn't find any
> documentation for TIOCL_SETSEL.
>
> And if userspace tries to set an invalid selection (e.g. by selecting row 0),
> my patch now returns -EINVAL, while it returned success before.
>
>> And the code below should handle this just fine, right:
>>>       v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
>>>       v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
>>>       v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
>
> It "handles it fine" in the sense that it can cope with the
> input and will not crash.
> But it returns (maybe?) unexpected results...

After some more thinking maybe you are right.
In case a user provided invalid values in the past, simply an unexpected
selection was set, but nothing broke.
Since the patch doesn't fix any critical issue, we could just drop this patch
and leave it as is.

Helge

Re: [PATCH] tty: vt: selection: Add check for valid tiocl_selection values

Posted by Jiri Slaby 3 years, 8 months ago

On 04. 08. 22, 10:44, Helge Deller wrote:
> On 8/4/22 09:15, Helge Deller wrote:
>> Hello Jiri,
>>
>> Thanks for looking into this patch!
>>
>> On 8/4/22 07:47, Jiri Slaby wrote:
>>> On 30. 07. 22, 20:49, Helge Deller wrote:
>>>> The line and column numbers for the selection need to start at 1.
>>>> Add the checks to prevent invalid input.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> Reported-by: syzbot+14b0e8f3fd1612e35350@syzkaller.appspotmail.com
>>>>
>>>> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
>>>> index f7755e73696e..58692a9b4097 100644
>>>> --- a/drivers/tty/vt/selection.c
>>>> +++ b/drivers/tty/vt/selection.c
>>>> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>>>>            return 0;
>>>>        }
>>>>
>>>> +    if (!v->xs || !v->ys || !v->xe || !v->ye)
>>>> +        return -EINVAL;
>>>
>>> Hmm, I'm not sure about this. It potentially breaks userspace (by
>>> returning EINVAL now).
>>
>> Right.
>> According to the code below, my interpretation is that all xs/ys/xe/ye values
>> should be > 0. But of course I might be wrong on this, as I didn't find any
>> documentation for TIOCL_SETSEL.
>>
>> And if userspace tries to set an invalid selection (e.g. by selecting row 0),
>> my patch now returns -EINVAL, while it returned success before.
>>
>>> And the code below should handle this just fine, right:
>>>>        v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
>>>>        v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
>>>>        v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
>>
>> It "handles it fine" in the sense that it can cope with the
>> input and will not crash.
>> But it returns (maybe?) unexpected results...
> 
> After some more thinking maybe you are right.
> In case a user provided invalid values in the past, simply an unexpected
> selection was set, but nothing broke.
> Since the patch doesn't fix any critical issue, we could just drop this patch
> and leave it as is.

We can still do a trial and revert it if something breaks... It's just 
that _noone_ knows with all this undocumented stuff ;).

But in fact, 0 currently means full row/column. Isn't it on purpose?

Today, we are out of luck, codesearch.debian.net gives no clue about users:
https://codesearch.debian.net/search?q=%5CbTIOCL_SETSEL%5Cb&literal=0

thanks,
-- 
js
suse labs

Re: [PATCH] tty: vt: selection: Add check for valid tiocl_selection values

Posted by Adam Borowski 3 years, 8 months ago

On Thu, Aug 04, 2022 at 11:22:26AM +0200, Jiri Slaby wrote:
> On 04. 08. 22, 10:44, Helge Deller wrote:
> > On 8/4/22 09:15, Helge Deller wrote:
> > > On 8/4/22 07:47, Jiri Slaby wrote:
> > > > On 30. 07. 22, 20:49, Helge Deller wrote:
> > > > > The line and column numbers for the selection need to start at 1.
> > > > > Add the checks to prevent invalid input.

> > > > > --- a/drivers/tty/vt/selection.c
> > > > > +++ b/drivers/tty/vt/selection.c
> > > > > @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
> > > > > +    if (!v->xs || !v->ys || !v->xe || !v->ye)
> > > > > +        return -EINVAL;
> > > > 
> > > > Hmm, I'm not sure about this. It potentially breaks userspace (by
> > > > returning EINVAL now).

> We can still do a trial and revert it if something breaks... It's just that
> _noone_ knows with all this undocumented stuff ;).
> 
> But in fact, 0 currently means full row/column. Isn't it on purpose?
> 
> Today, we are out of luck, codesearch.debian.net gives no clue about users:
> https://codesearch.debian.net/search?q=%5CbTIOCL_SETSEL%5Cb&literal=0

That's because the macro is undocumented.

"man ioctl_console" says:
       TIOCLINUX, subcode=2
              Set selection.  argp points to a [...]

thus everyone writes it as a number.

You'd need to grep for TIOCLINUX; there's not that many references to
check...


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀
⣾⠁⢠⠒⠀⣿⡁ Say what you want about Adolf, at least he was the man who
⢿⡄⠘⠷⠚⠋⠀ killed Hitler.  Your turn, Vlad!
⠈⠳⣄⠀⠀⠀⠀