Re: [PATCH] kernel: sys: fix potential Spectre v1

[Gustavo A. R. Silva]

On 05/20/2018 07:50 PM, Gustavo A. R. Silva wrote:
> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
> > > > #ifndef sanitize_index_nospec
> > > > inline bool sanitize_index_nospec(unsigned long *index,
> > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long size)
> > > > {
> > > > ÂÂÂÂÂÂÂÂ if (*index >= size)
> > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return false;
> > > > ÂÂÂÂÂÂÂÂ *index = array_index_nospec(*index, size);
> > > >
> > > > ÂÂÂÂÂÂÂÂ return true;
> > > > }
> > > > #endif
> > >
> > > I think this is fine in concept, we already do something similar in
> > > mpls_label_ok(). Perhaps call it validate_index_nospec() since
> > > validation is something that can fail, but sanitization in theory is
> > > something that can always succeed.
> >
> > OK. I got it.
> >
> > > However, the problem is the data type of the index. I expect you would
> > > need to do this in a macro and use typeof() if you wanted this to be
> > > generally useful, and also watch out for multiple usage of a macro
> > > argument. Is it still worth it at that point?
> >
> > Yeah. I think it is worth it. I'll work on this during the weekend and 
> > send a proper patch for review.
>
> Dan,
>
> What do you think about this first draft:
>
> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
> index e791ebc..6154183 100644
> --- a/include/linux/nospec.h
> +++ b/include/linux/nospec.h
> @@ -55,4 +55,16 @@ static inline unsigned long 
> array_index_mask_nospec(unsigned long index,
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  ÂÂÂÂÂÂÂ (typeof(_i)) (_i & _mask);ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  Â})
> +
> +#define validate_index_nospec(index, size)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +({ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂ typeof(index) *ptr = &(index);ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂ typeof(size) _s = (size);ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂ BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂ BUILD_BUG_ON(sizeof(_s) > sizeof(long));ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂ *ptr >= _s ? false :ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> +ÂÂÂÂÂÂ (*ptr = array_index_nospec(*ptr, _s) ? true : true);ÂÂÂÂÂÂÂÂÂÂ \

This actually should be:

((*ptr = array_index_nospec(*ptr, _s)) ? true : true);

> +})
>  Â#endif /* _LINUX_NOSPEC_H */
>
> Thanks
> --
> Gustavo