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

[Gustavo A. R. Silva]

On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
> > > >
> > >
> > > Oops, it seems I sent the wrong patch. The function would look like 
> > > this:
> > >
> > > #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.
>
> Thanks for the feedback.

BTW, I'm analyzing other cases, like the following:

bool foo(int x)
{
         if(!validate_index_nospec(&x))
                 return false;

         [...]

         return true;
}

int vulnerable(int x)
{
         if (!foo(x))
                 return -1;

         temp = array[x];

         [...]

}

Basically my doubt is how deep this barrier can be placed into the call 
chain in order to continue working.

Thanks
--
Gustavo