[PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO

[PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO

[Shreenidhi Shedi]

From: Shreenidhi Shedi <sshedi@vmware.com>

After finishing the loop, index value can be equal to LP_NO and lp_table
array is of size LP_NO, so this can end up in accessing an out of bound
address in lp_register function.

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
---
 drivers/char/lp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index 0e22e3b0a..d474d02b6 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -972,7 +972,7 @@ static void lp_attach(struct parport *port)
 			if (port_num[i] == -1)
 				break;

-		if (!lp_register(i, port))
+		if (i < LP_NO && !lp_register(i, port))
 			lp_count++;
 		break;

--
2.36.1

[PATCH 2/2] char: lp: remove redundant initialization of err

[Shreenidhi Shedi]

From: Shreenidhi Shedi <sshedi@vmware.com>

err is getting assigned with an appropriate value before returning,
hence this initialization is unnecessary.

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
---
 drivers/char/lp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index d474d02b6..536ceb111 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -1019,7 +1019,7 @@ static struct parport_driver lp_driver = {

 static int __init lp_init(void)
 {
-	int i, err = 0;
+	int i, err;

 	if (parport_nr[0] == LP_PARPORT_OFF)
 		return 0;
--
2.36.1

Re: [PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO

[Greg KH]

On Fri, Jun 03, 2022 at 06:30:39PM +0530, Shreenidhi Shedi wrote:
> From: Shreenidhi Shedi <sshedi@vmware.com>
> 
> After finishing the loop, index value can be equal to LP_NO and lp_table
> array is of size LP_NO, so this can end up in accessing an out of bound
> address in lp_register function.
> 
> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> ---
>  drivers/char/lp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 0e22e3b0a..d474d02b6 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -972,7 +972,7 @@ static void lp_attach(struct parport *port)
>  			if (port_num[i] == -1)
>  				break;
> 
> -		if (!lp_register(i, port))
> +		if (i < LP_NO && !lp_register(i, port))
>  			lp_count++;

How can this ever be needed?  Look at the check further up for the check
of lp_count which prevents this from every going too large.

So how can an address be accessed out of bound here?

thanks,

greg k-h

Re: [PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO

[Shreenidhi Shedi]

On 10/06/22 6:58 pm, Greg KH wrote:
> On Fri, Jun 03, 2022 at 06:30:39PM +0530, Shreenidhi Shedi wrote:
>> From: Shreenidhi Shedi <sshedi@vmware.com>
>>
>> After finishing the loop, index value can be equal to LP_NO and lp_table
>> array is of size LP_NO, so this can end up in accessing an out of bound
>> address in lp_register function.
>>
>> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
>> ---
>>  drivers/char/lp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
>> index 0e22e3b0a..d474d02b6 100644
>> --- a/drivers/char/lp.c
>> +++ b/drivers/char/lp.c
>> @@ -972,7 +972,7 @@ static void lp_attach(struct parport *port)
>>  			if (port_num[i] == -1)
>>  				break;
>>
>> -		if (!lp_register(i, port))
>> +		if (i < LP_NO && !lp_register(i, port))
>>  			lp_count++;
> 
> How can this ever be needed?  Look at the check further up for the check
> of lp_count which prevents this from every going too large.
> 
> So how can an address be accessed out of bound here?
> 
> thanks,
> 
> greg k-h

Thanks for the review. Assume lp_count is less than LP_NO now and we enter the for loop
and for some reason for loop exits after i reaching the value LP_NO and right after that 
we call lp_register() with i and I can be equal to LP_NO.

-- 
Shedi

Re: [PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO

[Greg KH]

On Fri, Jun 10, 2022 at 07:12:02PM +0530, Shreenidhi Shedi wrote:
> On 10/06/22 6:58 pm, Greg KH wrote:
> > On Fri, Jun 03, 2022 at 06:30:39PM +0530, Shreenidhi Shedi wrote:
> >> From: Shreenidhi Shedi <sshedi@vmware.com>
> >>
> >> After finishing the loop, index value can be equal to LP_NO and lp_table
> >> array is of size LP_NO, so this can end up in accessing an out of bound
> >> address in lp_register function.
> >>
> >> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> >> ---
> >>  drivers/char/lp.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> >> index 0e22e3b0a..d474d02b6 100644
> >> --- a/drivers/char/lp.c
> >> +++ b/drivers/char/lp.c
> >> @@ -972,7 +972,7 @@ static void lp_attach(struct parport *port)
> >>  			if (port_num[i] == -1)
> >>  				break;
> >>
> >> -		if (!lp_register(i, port))
> >> +		if (i < LP_NO && !lp_register(i, port))
> >>  			lp_count++;
> > 
> > How can this ever be needed?  Look at the check further up for the check
> > of lp_count which prevents this from every going too large.
> > 
> > So how can an address be accessed out of bound here?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Thanks for the review. Assume lp_count is less than LP_NO now and we enter the for loop
> and for some reason for loop exits after i reaching the value LP_NO

Wait, how can that happen?  That's what I am saying, the loop will never
reach that value from what I can tell.

Yes, this whole thing should be moved to something more sane like an
idr structure, but as-is, it seems correct to me.

Have you tested the code with that many devices to see if it really can
overflow?

thanks,

greg k-h

Re: [PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO

[Shreenidhi Shedi]

On 10/06/22 8:00 pm, Greg KH wrote:
> On Fri, Jun 10, 2022 at 07:12:02PM +0530, Shreenidhi Shedi wrote:
>> On 10/06/22 6:58 pm, Greg KH wrote:
>>> On Fri, Jun 03, 2022 at 06:30:39PM +0530, Shreenidhi Shedi wrote:
>>>> From: Shreenidhi Shedi <sshedi@vmware.com>
>>>>
>>>> After finishing the loop, index value can be equal to LP_NO and lp_table
>>>> array is of size LP_NO, so this can end up in accessing an out of bound
>>>> address in lp_register function.
>>>>
>>>> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
>>>> ---
>>>>  drivers/char/lp.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
>>>> index 0e22e3b0a..d474d02b6 100644
>>>> --- a/drivers/char/lp.c
>>>> +++ b/drivers/char/lp.c
>>>> @@ -972,7 +972,7 @@ static void lp_attach(struct parport *port)
>>>>  			if (port_num[i] == -1)
>>>>  				break;
>>>>
>>>> -		if (!lp_register(i, port))
>>>> +		if (i < LP_NO && !lp_register(i, port))
>>>>  			lp_count++;
>>>
>>> How can this ever be needed?  Look at the check further up for the check
>>> of lp_count which prevents this from every going too large.
>>>
>>> So how can an address be accessed out of bound here?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Thanks for the review. Assume lp_count is less than LP_NO now and we enter the for loop
>> and for some reason for loop exits after i reaching the value LP_NO
> 
> Wait, how can that happen?  That's what I am saying, the loop will never
> reach that value from what I can tell.
> 
> Yes, this whole thing should be moved to something more sane like an
> idr structure, but as-is, it seems correct to me.
> 
> Have you tested the code with that many devices to see if it really can
> overflow?
> 
> thanks,
> 
> greg k-h

No, I did not actually test it with real hardware but I ran a 
static analyzer check on this file and it also thinks the same.

```
Checking drivers/char/lp.c ...
drivers/char/lp.c:926:10: error: inconclusive: Array 'lp_table[8]' accessed at index 8, which is out of bounds. [arrayIndexOutOfBounds]
 lp_table[nr].dev = parport_register_dev_model(port, "lp",
         ^
drivers/char/lp.c:971:17: note: Assuming that condition 'i<8' is not redundant
  for (i = 0; i < LP_NO; i++)
                ^
drivers/char/lp.c:975:20: note: Calling function 'lp_register', 1st argument 'i' value is 8
  if (!lp_register(i, port))
                   ^
drivers/char/lp.c:926:10: note: Array index out of bounds
 lp_table[nr].dev = parport_register_dev_model(port, "lp",
         ^
drivers/char/lp.c:928:14: error: inconclusive: Array 'lp_table[8]' accessed at index 8, which is out of bounds. [arrayIndexOutOfBounds]
 if (lp_table[nr].dev == NULL)
             ^
drivers/char/lp.c:971:17: note: Assuming that condition 'i<8' is not redundant
  for (i = 0; i < LP_NO; i++)
                ^
drivers/char/lp.c:975:20: note: Calling function 'lp_register', 1st argument 'i' value is 8
  if (!lp_register(i, port))
                   ^
drivers/char/lp.c:928:14: note: Array index out of bounds
 if (lp_table[nr].dev == NULL)
```

-- 
Shedi

Re: [PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO

[Greg KH]

On Sat, Jun 11, 2022 at 11:15:26AM +0530, Shreenidhi Shedi wrote:
> On 10/06/22 8:00 pm, Greg KH wrote:
> > On Fri, Jun 10, 2022 at 07:12:02PM +0530, Shreenidhi Shedi wrote:
> >> On 10/06/22 6:58 pm, Greg KH wrote:
> >>> On Fri, Jun 03, 2022 at 06:30:39PM +0530, Shreenidhi Shedi wrote:
> >>>> From: Shreenidhi Shedi <sshedi@vmware.com>
> >>>>
> >>>> After finishing the loop, index value can be equal to LP_NO and lp_table
> >>>> array is of size LP_NO, so this can end up in accessing an out of bound
> >>>> address in lp_register function.
> >>>>
> >>>> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> >>>> ---
> >>>>  drivers/char/lp.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> >>>> index 0e22e3b0a..d474d02b6 100644
> >>>> --- a/drivers/char/lp.c
> >>>> +++ b/drivers/char/lp.c
> >>>> @@ -972,7 +972,7 @@ static void lp_attach(struct parport *port)
> >>>>  			if (port_num[i] == -1)
> >>>>  				break;
> >>>>
> >>>> -		if (!lp_register(i, port))
> >>>> +		if (i < LP_NO && !lp_register(i, port))
> >>>>  			lp_count++;
> >>>
> >>> How can this ever be needed?  Look at the check further up for the check
> >>> of lp_count which prevents this from every going too large.
> >>>
> >>> So how can an address be accessed out of bound here?
> >>>
> >>> thanks,
> >>>
> >>> greg k-h
> >>
> >> Thanks for the review. Assume lp_count is less than LP_NO now and we enter the for loop
> >> and for some reason for loop exits after i reaching the value LP_NO
> > 
> > Wait, how can that happen?  That's what I am saying, the loop will never
> > reach that value from what I can tell.
> > 
> > Yes, this whole thing should be moved to something more sane like an
> > idr structure, but as-is, it seems correct to me.
> > 
> > Have you tested the code with that many devices to see if it really can
> > overflow?
> > 
> > thanks,
> > 
> > greg k-h
> 
> No, I did not actually test it with real hardware but I ran a 
> static analyzer check on this file and it also thinks the same.

Please try it on real hardware.  static tools are not always the
smartest thing out there.

good luck!

greg k-h