Michael Ellerman <m...@ellerman.id.au> writes: > Nathan Lynch <nath...@linux.ibm.com> writes: >> >> 1. The patch sanitizes 'nargs' immediately before the call to memset(), >> but shouldn't that happen before 'nargs' is used as an input to >> copy_from_user()? > > I think the reasoning is that there's no way to exploit an out of bounds > value using copy_from_user(). But it's much easier to reason about if we > just do the sanitisation up front. > >> 2. If 'nargs' needs this treatment, then why wouldn't the user-supplied >> 'nret' and 'token' need them as well? 'nret' is used to index the >> same array as 'nargs'. And at least conceptually, 'token' is used to >> index a data structure (xarray) with array-like semantics (to be >> fair, this is a relatively recent development and was not the case >> when this change was submitted). > > I don't know exactly what smatch looks for when trying to detect these, > but I suspect it's a plain array access. Not sure why it doesn't > complain about nret, but I think it would be good to sanitise it as > well.
Agreed. I'm sending a new patch that does this. > token is different, at least in the above code, because it's not bounds > checked, so there's no bounds check to bypass. Right. > Though maybe there is one inside the rtas lookup code that should be > masked. In rtas_function_token()? I think it's OK... the handles passed to it are always build-time constants that are supposed to be within the bounds of the function table.
