From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752153AbeERWBb (ORCPT ); Fri, 18 May 2018 18:01:31 -0400 Received: from gateway31.websitewelcome.com ([192.185.143.35]:49672 "EHLO gateway31.websitewelcome.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752024AbeERWB2 (ORCPT ); Fri, 18 May 2018 18:01:28 -0400 X-Authority-Reason: nr=8 Subject: Re: [PATCH] kernel: sys: fix potential Spectre v1 To: Dan Williams Cc: Thomas Gleixner , Andrew Morton , Linux Kernel Mailing List , Alexei Starovoitov , Peter Zijlstra References: <20180515030038.GA11822@embeddedor.com> <20180515150859.1bccbd8d4543848b30fea859@linux-foundation.org> <50481b83-4c03-f354-bd11-cef7aecdd85f@embeddedor.com> <3d2e5771-c2c9-6e45-3e85-21c0bc86876e@embeddedor.com> From: "Gustavo A. R. Silva" Message-ID: Date: Fri, 18 May 2018 17:01:26 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - gator4166.hostgator.com X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - embeddedor.com X-BWhitelist: no X-Source-IP: 187.162.252.93 X-Source-L: No X-Exim-ID: 1fJnRH-002Ijx-9P X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 187-162-252-93.static.axtel.net ([192.168.15.106]) [187.162.252.93]:51162 X-Source-Auth: gustavo@embeddedor.com X-Email-Count: 4 X-Source-Cap: Z3V6aWRpbmU7Z3V6aWRpbmU7Z2F0b3I0MTY2Lmhvc3RnYXRvci5jb20= X-Local-Domain: yes Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/18/2018 04:45 PM, Dan Williams wrote: > On Fri, May 18, 2018 at 2:27 PM, Gustavo A. R. Silva > wrote: >> >> >> 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. > > This is broken you would need to pass the address of x into foo() > otherwise there may be speculation on the return value of foo. > Oh I see now. Just to double check, then something like the following would be broken too, because is basically the same as the code above, and well, it doesn't make much sense to store the value returned by macro array_index_nospec into x, correct?: bool foo(int x) { if(x >= MAX) return false; x = array_index_nospec(x, MAX); return true; } int vulnerable(int x) { if(!foo(x)) return -1; temp = array[x]; [...] } Thanks -- Gustavo