kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Input: joydev - prevent potential read overflow in ioctl
@ 2021-02-17  6:10 Dan Carpenter
  2021-02-19  3:17 ` Dmitry Torokhov
  2021-07-03 11:20 ` Denis Efremov
  0 siblings, 2 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-02-17  6:10 UTC (permalink / raw)
  To: Dmitry Torokhov, Stephen Kitt; +Cc: linux-input, kernel-janitors

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.

Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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;
@@ -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;
-- 
2.30.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl
  2021-02-17  6:10 [PATCH] Input: joydev - prevent potential read overflow in ioctl Dan Carpenter
@ 2021-02-19  3:17 ` Dmitry Torokhov
  2021-02-19  8:32   ` Dan Carpenter
  2021-07-03 11:20 ` Denis Efremov
  1 sibling, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2021-02-19  3:17 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Stephen Kitt, linux-input, kernel-janitors

Hi Dan,

On Wed, Feb 17, 2021 at 09:10:15AM +0300, Dan Carpenter wrote:
> 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.
> 
> Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thank you for the patch.

> ---
>  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;
> @@ -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++) {


I think we also need to make sure that len is even. Do you mind if I add
the check at the top of the function to return -EINVAL if it is odd?

>  		if (keypam[i] > KEY_MAX || keypam[i] < BTN_MISC) {
>  			retval = -EINVAL;
>  			goto out;
> -- 
> 2.30.0
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl
  2021-02-19  3:17 ` Dmitry Torokhov
@ 2021-02-19  8:32   ` Dan Carpenter
  2021-02-19 18:54     ` Dmitry Torokhov
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-02-19  8:32 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Stephen Kitt, linux-input, kernel-janitors

On Thu, Feb 18, 2021 at 07:17:44PM -0800, Dmitry Torokhov wrote:
> Hi Dan,
> 
> On Wed, Feb 17, 2021 at 09:10:15AM +0300, Dan Carpenter wrote:
> > 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.
> > 
> > Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Thank you for the patch.
> 
> > ---
> >  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;
> > @@ -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++) {
> 
> 
> I think we also need to make sure that len is even. Do you mind if I add
> the check at the top of the function to return -EINVAL if it is odd?

Yes.  You are right.

Btw, thank you for fixing my patches, but feel free to just ask me to
resend if that's easier for you.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl
  2021-02-19  8:32   ` Dan Carpenter
@ 2021-02-19 18:54     ` Dmitry Torokhov
  0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2021-02-19 18:54 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Stephen Kitt, linux-input, kernel-janitors

On Fri, Feb 19, 2021 at 11:32:15AM +0300, Dan Carpenter wrote:
> On Thu, Feb 18, 2021 at 07:17:44PM -0800, Dmitry Torokhov wrote:
> > Hi Dan,
> > 
> > On Wed, Feb 17, 2021 at 09:10:15AM +0300, Dan Carpenter wrote:
> > > 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.
> > > 
> > > Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > Thank you for the patch.
> > 
> > > ---
> > >  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;
> > > @@ -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++) {
> > 
> > 
> > I think we also need to make sure that len is even. Do you mind if I add
> > the check at the top of the function to return -EINVAL if it is odd?
> 
> Yes.  You are right.
> 
> Btw, thank you for fixing my patches, but feel free to just ask me to
> resend if that's easier for you.

It is not really fixing your patch, rather addressing another issue
while we are there. Some might even say it could have been a separate
patch.

Anyway, for small fixups like this I feel it's less noise on the lists
without extra resubmissions, that is why I often do them on my side.

Thanks!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl
  2021-02-17  6:10 [PATCH] Input: joydev - prevent potential read overflow in ioctl Dan Carpenter
  2021-02-19  3:17 ` Dmitry Torokhov
@ 2021-07-03 11:20 ` Denis Efremov
  2021-07-03 12:43   ` Dan Carpenter
  1 sibling, 1 reply; 7+ messages in thread
From: Denis Efremov @ 2021-07-03 11:20 UTC (permalink / raw)
  To: Dan Carpenter, Dmitry Torokhov, Stephen Kitt; +Cc: linux-input, kernel-janitors

Hi,

CVE-2021-3612 was assigned to this patch.


On 2/17/21 9:10 AM, Dan Carpenter wrote:
> 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.

> 
> Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  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;


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl
  2021-07-03 11:20 ` Denis Efremov
@ 2021-07-03 12:43   ` Dan Carpenter
  2021-07-03 14:34     ` Denis Efremov
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-07-03 12:43 UTC (permalink / raw)
  To: Denis Efremov
  Cc: Dmitry Torokhov, Stephen Kitt, linux-input, kernel-janitors,
	Alexander Larkin

On Sat, Jul 03, 2021 at 02:20:13PM +0300, Denis Efremov wrote:
> Hi,
> 
> CVE-2021-3612 was assigned to this patch.
> 
> 
> On 2/17/21 9:10 AM, Dan Carpenter wrote:
> > 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/

regards,
dan carpenter

> > 
> > Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  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;
> 
> 
> > @@ -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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl
  2021-07-03 12:43   ` Dan Carpenter
@ 2021-07-03 14:34     ` Denis Efremov
  0 siblings, 0 replies; 7+ messages in thread
From: Denis Efremov @ 2021-07-03 14:34 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Dmitry Torokhov, Stephen Kitt, linux-input, kernel-janitors,
	Alexander Larkin, Murray McAllister

On 7/3/21 3:43 PM, Dan Carpenter wrote:
> On Sat, Jul 03, 2021 at 02:20:13PM +0300, Denis Efremov wrote:
>> 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.

>>
>>
>> On 2/17/21 9:10 AM, Dan Carpenter wrote:
>>> 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/
> 
> regards,
> dan carpenter
> 
>>>
>>> Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>>  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;
>>
>>
>>> @@ -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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-07-03 14:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-17  6:10 [PATCH] Input: joydev - prevent potential read overflow in ioctl Dan Carpenter
2021-02-19  3:17 ` Dmitry Torokhov
2021-02-19  8:32   ` Dan Carpenter
2021-02-19 18:54     ` Dmitry Torokhov
2021-07-03 11:20 ` Denis Efremov
2021-07-03 12:43   ` Dan Carpenter
2021-07-03 14:34     ` Denis Efremov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).