On Tue, Mar 28, 2023 at 05:55:40PM +0300, Dan Carpenter wrote: > On Fri, Jan 20, 2023 at 08:22:06AM +0300, Dan Carpenter wrote: > > On Thu, Jan 19, 2023 at 11:21:22AM -0500, Jeff Moyer wrote: > > > Dan Carpenter <error27@xxxxxxxxx> writes: > > > > > > > The concern here would be that "family" is negative and we pass a > > > > negative value to test_bit() resulting in an out of bounds read > > > > and potentially a crash. > > > > > > I don't see how this can happen. Do you have a particular scenario in > > > mind? > > > > > > > This is from static analysis. My main thinking was: > > > > 1) The static checker says that this comes from the user. > > 2) Every upper bounds check should have a lower bounds check. > > 3) family is passed to array_index_nospec() so we must not trust it. > > > > But looking closer today here is what the checker is concerned about: > > > > func = cmd_to_func(nfit_mem, cmd, call_pkg, &family); > > > > Assume "nfit_mem" is NULL but "call_pkg" is non NULL (user input from > > __nd_ioctl() or ars_get_status(). In that case family is unchecked user > > input. > > > > But probably, it's not possible for nfit_mem to be NULL in those caller > > functions? > > Did we ever figure out if it's possible for nfit_mem to be NULL? Another idea is I could send this patch as a static checker fix instead of a security vulnerability. That way we would be safe going forward regardless. regards, dan carpenter
