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

[Greg KH <gregkh@linuxfoundation.org>]

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