Only the return value of copy_to_user() is checked in con_get_unimap().
Do the same for put_user() of the count too.
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/consolemap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 831450f2bfd1..92b5dddb00d9 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
console_unlock();
if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
ret = -EFAULT;
- put_user(ect, uct);
+ if (put_user(ect, uct))
+ ret = -EFAULT;
kvfree(unilist);
return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
}
--
2.36.1
On Tue, 7 Jun 2022, Jiri Slaby wrote:
> Only the return value of copy_to_user() is checked in con_get_unimap().
> Do the same for put_user() of the count too.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> ---
> drivers/tty/vt/consolemap.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 831450f2bfd1..92b5dddb00d9 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
> console_unlock();
> if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
> ret = -EFAULT;
> - put_user(ect, uct);
> + if (put_user(ect, uct))
> + ret = -EFAULT;
> kvfree(unilist);
> return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
> }
>
Doesn't this fix something?
--
i.
On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Jiri Slaby wrote:
>
>> Only the return value of copy_to_user() is checked in con_get_unimap().
>> Do the same for put_user() of the count too.
>>
>> Signed-off-by: Jiri Slaby <[email protected]>
>> ---
>> drivers/tty/vt/consolemap.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 831450f2bfd1..92b5dddb00d9 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
>> console_unlock();
>> if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>> ret = -EFAULT;
>> - put_user(ect, uct);
>> + if (put_user(ect, uct))
>> + ret = -EFAULT;
>> kvfree(unilist);
>> return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>> }
>>
>
> Doesn't this fix something?
If you mean a Fixes tag, this is pre-git.
If you mean a bug, well, likely yes, users now get informed. But I don't
think anyone cares ;). But who knows, maybe we will start seeing
userspace failures now (as they might not provide writable count field
-- unlikely). That's one of the reasons why I did this as a separate
commit. Let's see if are going to revert this or not...
thanks,
--
js
suse labs
On 08. 06. 22, 10:02, David Laight wrote:
> From: Jiri Slaby
>> Sent: 07 June 2022 11:49
>>
>> Only the return value of copy_to_user() is checked in con_get_unimap().
>> Do the same for put_user() of the count too.
>>
>> Signed-off-by: Jiri Slaby <[email protected]>
>> ---
>> drivers/tty/vt/consolemap.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 831450f2bfd1..92b5dddb00d9 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
>> console_unlock();
>> if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>> ret = -EFAULT;
>> - put_user(ect, uct);
>> + if (put_user(ect, uct))
>> + ret = -EFAULT;
>> kvfree(unilist);
>> return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>> }
>
> How is the user expected to check the result of this code?
>
> AFAICT -ENOMEM is returned if either kmalloc() fails or
> the user buffer is too short?
> Looks pretty hard to detect which.
Agreed. The code is far from perfect. We might try to return ENOSPC and
watch what breaks. (And decouple the double "?:" operator as it makes
things only worse.)
> I've not looked at the effect of all the patches, but setting
> 'ret = -ENOMEM' and breaking the loop when the array is too
> small would simplify things.
Note that the patches try NOT to change the behavior in any way. If they
do, it's likely a bug. They are first front cleanup. Definitely more to
come. Either from me, or others -- patches welcome ;).
thanks,
--
js
suse labs
On Wed, 8 Jun 2022, Jiri Slaby wrote:
> On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> > On Tue, 7 Jun 2022, Jiri Slaby wrote:
> >
> > > Only the return value of copy_to_user() is checked in con_get_unimap().
> > > Do the same for put_user() of the count too.
> > >
> > > Signed-off-by: Jiri Slaby <[email protected]>
> > > ---
> > > drivers/tty/vt/consolemap.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> > > index 831450f2bfd1..92b5dddb00d9 100644
> > > --- a/drivers/tty/vt/consolemap.c
> > > +++ b/drivers/tty/vt/consolemap.c
> > > @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct,
> > > ushort __user *uct,
> > > console_unlock();
> > > if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
> > > ret = -EFAULT;
> > > - put_user(ect, uct);
> > > + if (put_user(ect, uct))
> > > + ret = -EFAULT;
> > > kvfree(unilist);
> > > return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
> > > }
> > >
> >
> > Doesn't this fix something?
>
> If you mean a Fixes tag, this is pre-git.
>
> If you mean a bug, well, likely yes, users now get informed. But I don't think
> anyone cares ;).
Yes, I meant Fixes tag but I guess it's not important.
> But who knows, maybe we will start seeing userspace failures
> now (as they might not provide writable count field -- unlikely). That's one
> of the reasons why I did this as a separate commit. Let's see if are going to
> revert this or not...
Ok.
--
i.
On Wed, Jun 8, 2022 at 10:56 AM Jiri Slaby <[email protected]> wrote:
> On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> > On Tue, 7 Jun 2022, Jiri Slaby wrote:
...
> > Doesn't this fix something?
>
> If you mean a Fixes tag, this is pre-git.
You may use history.git [1] for pregit SHAs.
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 08, 2022 at 12:38:49PM +0200, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 10:56 AM Jiri Slaby <[email protected]> wrote:
> > On 07. 06. 22, 16:19, Ilpo J?rvinen wrote:
> > > On Tue, 7 Jun 2022, Jiri Slaby wrote:
>
> ...
>
> > > Doesn't this fix something?
> >
> > If you mean a Fixes tag, this is pre-git.
>
> You may use history.git [1] for pregit SHAs.
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
Please don't, it just does not matter.
thanks,
greg k-h
On 08. 06. 22, 10:11, Jiri Slaby wrote:
> On 08. 06. 22, 10:02, David Laight wrote:
>> From: Jiri Slaby
>>> Sent: 07 June 2022 11:49
>>>
>>> Only the return value of copy_to_user() is checked in con_get_unimap().
>>> Do the same for put_user() of the count too.
>>>
>>> Signed-off-by: Jiri Slaby <[email protected]>
>>> ---
>>> drivers/tty/vt/consolemap.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>>> index 831450f2bfd1..92b5dddb00d9 100644
>>> --- a/drivers/tty/vt/consolemap.c
>>> +++ b/drivers/tty/vt/consolemap.c
>>> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct,
>>> ushort __user *uct,
>>> console_unlock();
>>> if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>>> ret = -EFAULT;
>>> - put_user(ect, uct);
>>> + if (put_user(ect, uct))
>>> + ret = -EFAULT;
>>> kvfree(unilist);
>>> return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>>> }
>>
>> How is the user expected to check the result of this code?
>>
>> AFAICT -ENOMEM is returned if either kmalloc() fails or
>> the user buffer is too short?
>> Looks pretty hard to detect which.
>
> Agreed. The code is far from perfect. We might try to return ENOSPC and
> watch what breaks.
brltty and kbd (see below) would break at least:
https://sources.debian.org/src/brltty/6.4-6/Drivers/Screen/Linux/screen.c/#L875
brltty apparently relies exactly on ENOMEM, increases buffer if that
error is returned, and retries.
So I don't think we can change that ENOMEM to anything else.
>> I've not looked at the effect of all the patches, but setting
>> 'ret = -ENOMEM' and breaking the loop when the array is too
>> small would simplify things.
That would break kbd for example:
https://sources.debian.org/src/kbd/2.3.0-3/src/libkfont/kdmapop.c/?hl=154#L159
GIO_UNIMAP is called with zero count to actually find out the count...
So apart from the original patch which checks the return value of
put_user, we cannot do anything else. (Except decoupling the "?:" to
make it more readable.)
thanks,
--
js
suse labs