Re: [PATCH] powerpc/rtas: Prevent Spectre v1 gadget construction in sys_rtas()

[Breno Leitao <leitao@debian.org>]

On Fri, May 31, 2024 at 11:45:48AM -0500, Nathan Lynch wrote:
> Breno Leitao <leitao@debian.org> writes:
> 
> > On Thu, May 30, 2024 at 07:44:12PM -0500, Nathan Lynch via B4 Relay wrote:
> >> From: Nathan Lynch <nathanl@linux.ibm.com>

> >> +	nargs = array_index_nospec(nargs, ARRAY_SIZE(args.args));
> >> +	nret = array_index_nospec(nret, ARRAY_SIZE(args.args) - nargs);
> >
> > On an unrelated note, can nargs and nret are integers and could be
> > eventually negative. Is this a valid use case?
> 
> No, it's not valid for a caller to provide negative nargs or nret. I
> convinced myself that this bounds check:
> 
> 	nargs = be32_to_cpu(args.nargs);
> 	nret  = be32_to_cpu(args.nret);
> 
> 	if (nargs >= ARRAY_SIZE(args.args)
> 	    || nret > ARRAY_SIZE(args.args)
> 	    || nargs + nret > ARRAY_SIZE(args.args))
> 		return -EINVAL;
> 
> rejects negative values of nargs or nret due to C's "usual arithmetic
> conversions", where nargs and nret are implicitly converted to size_t
> for the comparisons.
> 
> However I don't see any value in keeping them as signed int. I have some
> changes in progress in this area and I'll plan on making these unsigned.

yea, I think it will help to make this code easier to read/review.

Thanks again for fixing it.