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 <jslaby@xxxxxxx>
---
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
