Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl

On 7/3/21 3:43 PM, Dan Carpenter wrote:

On Sat, Jul 03, 2021 at 02:20:13PM +0300, Denis Efremov wrote:

quoted

Hi,

CVE-2021-3612 was assigned to this patch.

Oh, sorry CVE-2021-3612 assigned not to:
182d679b2298 Input: joydev - prevent potential read overflow in ioctl

But for not yet applied fix
https://access.redhat.com/security/cve/cve-2021-3612:

An out-of-bounds memory write flaw was found in the Linux kernel’s
joystick devices subsystem, in the way the user calls ioctl JSIOCSBTNMAP. 

So it appears that CVE is assigned but the fix is not in the mainline yet.
Maybe someone could explicitly list the CVE id in a commit message then.

quoted

On 2/17/21 9:10 AM, Dan Carpenter wrote:

quoted

The problem here is that "len" might be less than "joydev->nabs" so the
loops which verfy abspam[i] and keypam[] might read beyond the buffer.

The added check looks insufficient to me. There are second loops in these
functions with unpatched "i < joydev->nabs;" and "i < joydev->nkey;" checks.

Thanks, Denis.

You're right.  The fix isn't complete.  We discussed this in a different
thread but I sort of assumed it was dealt with and didn't follow up.  :/

https://lore.kernel.org/lkml/20210620120030.1513655-1-avlarkin82@gmail.com/ (local)

regards,
dan carpenter

quoted

quoted

Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
Signed-off-by: Dan Carpenter <redacted>
---
 drivers/input/joydev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index a2b5fbba2d3b..750f4513fe20 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c

@@ -456,7 +456,7 @@ static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
 	if (IS_ERR(abspam))
 		return PTR_ERR(abspam);
 
-	for (i = 0; i < joydev->nabs; i++) {
+	for (i = 0; i < len && i < joydev->nabs; i++) {
 		if (abspam[i] > ABS_MAX) {
 			retval = -EINVAL;
 			goto out;

        memcpy(joydev->abspam, abspam, len);

        for (i = 0; i < joydev->nabs; i++) // <== HERE
                joydev->absmap[joydev->abspam[i]] = i;

quoted

@@ -487,7 +487,7 @@ static int joydev_handle_JSIOCSBTNMAP(struct joydev *joydev,
 	if (IS_ERR(keypam))
 		return PTR_ERR(keypam);
 
-	for (i = 0; i < joydev->nkey; i++) {
+	for (i = 0; i < (len / 2) && i < joydev->nkey; i++) {
 		if (keypam[i] > KEY_MAX || keypam[i] < BTN_MISC) {
 			retval = -EINVAL;
 			goto out;

        memcpy(joydev->keypam, keypam, len);

        for (i = 0; i < joydev->nkey; i++) // <== HERE
                joydev->keymap[keypam[i] - BTN_MISC] = i;


Also at the beginning of joydev_handle_JSIOCSAXMAP() there is a
	len = min(len, sizeof(joydev->abspam));
	abspam = memdup_user(argp, len);

Maybe we can call min(len, joydev->nabs) instead or even min3() and
use only len in the for loops then? Same for joydev_handle_JSIOCSBTNMAP.

Thanks,
Denis