Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <35e860bb-c76c-ca5f-3f48-2bf6cb798689@gmx.de>
Date:   Thu, 4 Aug 2022 09:15:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] tty: vt: selection: Add check for valid tiocl_selection
 values
Content-Language: en-US
To:     Jiri Slaby <jirislaby@kernel.org>,
        Khalid Masum <khalid.masum.92@gmail.com>,
        syzbot <syzbot+14b0e8f3fd1612e35350@syzkaller.appspotmail.com>,
        dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
References: <000000000000bbdd0405d120c155@google.com>
 <20220729065139.6529-1-khalid.masum.92@gmail.com>
 <eb4a26aa-da30-ceee-7d27-c1e902dd4218@gmx.de> <YuV9PybMPgc83Jis@p100>
 <1eb62346-304b-54d5-8a62-8a35888d51bd@kernel.org>
From:   Helge Deller <deller@gmx.de>
In-Reply-To: <1eb62346-304b-54d5-8a62-8a35888d51bd@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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,
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>>
>> +=C2=A0=C2=A0=C2=A0 if (!v->xs || !v->ys || !v->xe || !v->ye)
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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 val=
ues
should be > 0. But of course I might be wrong on this, as I didn't find an=
y
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 =3D min_t(u16, v->xs - 1, vc->vc_cols - 1);
>>       v->ys =3D min_t(u16, v->ys - 1, vc->vc_rows - 1);
>>       v->xe =3D 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