[PATCH] mips: export icache_flush_range

[PATCH] mips: export icache_flush_range

[Kees Cook]

The lkdtm module performs tests against executable memory ranges, so
it needs to flush the icache for proper behaviors. Other architectures
already export this, so do the same for MIPS.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
This is currently untested! I'm building a MIPS cross-compiler now...
If someone can validate this fixes the build when lkdtm is a module,
that would be appreciated. :)
---
 arch/mips/mm/cache.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index fde7e56d13fe..b3f1df13d9f6 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr, int size);
 void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
 
 EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
+EXPORT_SYMBOL_GPL(flush_icache_range);
 
 /* MIPS specific cache operations */
 void (*flush_cache_sigtramp)(unsigned long addr);
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] mips: export icache_flush_range

[Kees Cook]

On Sat, Mar 22, 2014 at 9:47 AM, Kees Cook <keescook@chromium.org> wrote:
> The lkdtm module performs tests against executable memory ranges, so
> it needs to flush the icache for proper behaviors. Other architectures
> already export this, so do the same for MIPS.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This is currently untested! I'm building a MIPS cross-compiler now...
> If someone can validate this fixes the build when lkdtm is a module,
> that would be appreciated. :)

Okay, now tested. I reproduced the failure and this patch fixes it. :)

-Kees

> ---
>  arch/mips/mm/cache.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
> index fde7e56d13fe..b3f1df13d9f6 100644
> --- a/arch/mips/mm/cache.c
> +++ b/arch/mips/mm/cache.c
> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr, int size);
>  void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>
>  EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
> +EXPORT_SYMBOL_GPL(flush_icache_range);
>
>  /* MIPS specific cache operations */
>  void (*flush_cache_sigtramp)(unsigned long addr);
> --
> 1.7.9.5
>
>
> --
> Kees Cook
> Chrome OS Security



-- 
Kees Cook
Chrome OS Security

Re: [PATCH] mips: export icache_flush_range

[Paul Gortmaker]

On 14-03-22 03:05 PM, Kees Cook wrote:
> On Sat, Mar 22, 2014 at 9:47 AM, Kees Cook <keescook@chromium.org> wrote:
>> The lkdtm module performs tests against executable memory ranges, so
>> it needs to flush the icache for proper behaviors. Other architectures
>> already export this, so do the same for MIPS.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> This is currently untested! I'm building a MIPS cross-compiler now...
>> If someone can validate this fixes the build when lkdtm is a module,
>> that would be appreciated. :)
> 
> Okay, now tested. I reproduced the failure and this patch fixes it. :)

Just checking if this happened to fall through the cracks.
The most recent (Apr4) linux-next build for mips still fails
with this error.

http://kisskb.ellerman.id.au/kisskb/buildresult/10877159/

Paul.
--

> 
> -Kees
> 
>> ---
>>  arch/mips/mm/cache.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
>> index fde7e56d13fe..b3f1df13d9f6 100644
>> --- a/arch/mips/mm/cache.c
>> +++ b/arch/mips/mm/cache.c
>> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr, int size);
>>  void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>>
>>  EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
>> +EXPORT_SYMBOL_GPL(flush_icache_range);
>>
>>  /* MIPS specific cache operations */
>>  void (*flush_cache_sigtramp)(unsigned long addr);
>> --
>> 1.7.9.5
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
> 
> 
> 

Re: [PATCH] mips: export icache_flush_range

[Kees Cook]

On Mon, Apr 7, 2014 at 6:58 AM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> On 14-03-22 03:05 PM, Kees Cook wrote:
>> On Sat, Mar 22, 2014 at 9:47 AM, Kees Cook <keescook@chromium.org> wrote:
>>> The lkdtm module performs tests against executable memory ranges, so
>>> it needs to flush the icache for proper behaviors. Other architectures
>>> already export this, so do the same for MIPS.
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> This is currently untested! I'm building a MIPS cross-compiler now...
>>> If someone can validate this fixes the build when lkdtm is a module,
>>> that would be appreciated. :)
>>
>> Okay, now tested. I reproduced the failure and this patch fixes it. :)
>
> Just checking if this happened to fall through the cracks.
> The most recent (Apr4) linux-next build for mips still fails
> with this error.
>
> http://kisskb.ellerman.id.au/kisskb/buildresult/10877159/

Ralf, you acked this offlist, but someone needs to pull this into the tree?

On Mar 24, Ralf Baechle wrote:
> Acked-by: Ralf Baechle <ralf@linux-mips.org>

Thanks!

-Kees

>
> Paul.
> --
>
>>
>> -Kees
>>
>>> ---
>>>  arch/mips/mm/cache.c |    1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
>>> index fde7e56d13fe..b3f1df13d9f6 100644
>>> --- a/arch/mips/mm/cache.c
>>> +++ b/arch/mips/mm/cache.c
>>> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr, int size);
>>>  void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>>>
>>>  EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
>>> +EXPORT_SYMBOL_GPL(flush_icache_range);
>>>
>>>  /* MIPS specific cache operations */
>>>  void (*flush_cache_sigtramp)(unsigned long addr);
>>> --
>>> 1.7.9.5
>>>
>>>
>>> --
>>> Kees Cook
>>> Chrome OS Security
>>
>>
>>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH] mips: export icache_flush_range

[Sergei Shtylyov]

Hello.

On 03/22/2014 06:47 PM, Kees Cook wrote:

> The lkdtm module performs tests against executable memory ranges, so
> it needs to flush the icache for proper behaviors. Other architectures
> already export this, so do the same for MIPS.

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This is currently untested! I'm building a MIPS cross-compiler now...
> If someone can validate this fixes the build when lkdtm is a module,
> that would be appreciated. :)
> ---
>   arch/mips/mm/cache.c |    1 +
>   1 file changed, 1 insertion(+)

> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
> index fde7e56d13fe..b3f1df13d9f6 100644
> --- a/arch/mips/mm/cache.c
> +++ b/arch/mips/mm/cache.c
> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr, int size);
>   void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>
>   EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
> +EXPORT_SYMBOL_GPL(flush_icache_range);

    Have you run this thru scripts/checkpatch.pl? It would have told you that 
an export should immediately follow the corresponding function body, AFAIK.

WBR, Sergei

Re: [PATCH] mips: export icache_flush_range

[Sergei Shtylyov]

On 03/23/2014 12:45 AM, Sergei Shtylyov wrote:

>> The lkdtm module performs tests against executable memory ranges, so
>> it needs to flush the icache for proper behaviors. Other architectures
>> already export this, so do the same for MIPS.

>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> This is currently untested! I'm building a MIPS cross-compiler now...
>> If someone can validate this fixes the build when lkdtm is a module,
>> that would be appreciated. :)
>> ---
>>   arch/mips/mm/cache.c |    1 +
>>   1 file changed, 1 insertion(+)

>> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
>> index fde7e56d13fe..b3f1df13d9f6 100644
>> --- a/arch/mips/mm/cache.c
>> +++ b/arch/mips/mm/cache.c
>> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr, int
>> size);
>>   void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>>
>>   EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
>> +EXPORT_SYMBOL_GPL(flush_icache_range);

>     Have you run this thru scripts/checkpatch.pl? It would have told you that
> an export should immediately follow the corresponding function body, AFAIK.

    Hm, it doesn't now but definitely used to...

WBR, Sergei

Re: [PATCH] mips: export icache_flush_range

[Sergei Shtylyov]

On 03/23/2014 12:48 AM, Sergei Shtylyov wrote:

>>> The lkdtm module performs tests against executable memory ranges, so
>>> it needs to flush the icache for proper behaviors. Other architectures
>>> already export this, so do the same for MIPS.

>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> This is currently untested! I'm building a MIPS cross-compiler now...
>>> If someone can validate this fixes the build when lkdtm is a module,
>>> that would be appreciated. :)
>>> ---
>>>   arch/mips/mm/cache.c |    1 +
>>>   1 file changed, 1 insertion(+)

>>> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
>>> index fde7e56d13fe..b3f1df13d9f6 100644
>>> --- a/arch/mips/mm/cache.c
>>> +++ b/arch/mips/mm/cache.c
>>> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr, int
>>> size);
>>>   void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>>>
>>>   EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
>>> +EXPORT_SYMBOL_GPL(flush_icache_range);

>>     Have you run this thru scripts/checkpatch.pl? It would have told you that
>> an export should immediately follow the corresponding function body, AFAIK.

>     Hm, it doesn't now but definitely used to...

    Decided to check Documentation/CodingStyle, and it still codifies this:

In source files, separate functions with one blank line.  If the function is
exported, the EXPORT* macro for it should follow immediately after the closing
function brace line.  E.g.:

int system_is_up(void)
{
         return system_state == SYSTEM_RUNNING;
}
EXPORT_SYMBOL(system_is_up);

WBR, Sergei

Re: [PATCH] mips: export icache_flush_range

[Kees Cook]

On Sat, Mar 22, 2014 at 3:54 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 03/23/2014 12:48 AM, Sergei Shtylyov wrote:
>
>>>> The lkdtm module performs tests against executable memory ranges, so
>>>> it needs to flush the icache for proper behaviors. Other architectures
>>>> already export this, so do the same for MIPS.
>
>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>> This is currently untested! I'm building a MIPS cross-compiler now...
>>>> If someone can validate this fixes the build when lkdtm is a module,
>>>> that would be appreciated. :)
>>>> ---
>>>>   arch/mips/mm/cache.c |    1 +
>>>>   1 file changed, 1 insertion(+)
>
>
>>>> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
>>>> index fde7e56d13fe..b3f1df13d9f6 100644
>>>> --- a/arch/mips/mm/cache.c
>>>> +++ b/arch/mips/mm/cache.c
>>>> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr,
>>>> int
>>>> size);
>>>>   void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>>>>
>>>>   EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
>>>> +EXPORT_SYMBOL_GPL(flush_icache_range);
>
>
>>>     Have you run this thru scripts/checkpatch.pl? It would have told you
>>> that
>>> an export should immediately follow the corresponding function body,
>>> AFAIK.
>
>
>>     Hm, it doesn't now but definitely used to...
>
>
>    Decided to check Documentation/CodingStyle, and it still codifies this:
>
> In source files, separate functions with one blank line.  If the function is
> exported, the EXPORT* macro for it should follow immediately after the
> closing
> function brace line.  E.g.:
>
> int system_is_up(void)
> {
>         return system_state == SYSTEM_RUNNING;
> }
> EXPORT_SYMBOL(system_is_up);
>
> WBR, Sergei

Yup, thanks. I know the style, but it seemed from the source file that
the style was to declare the function pointers in bulk, and then
explicitly export them in the next section, so I continued that style.
For example:

void (*flush_cache_sigtramp)(unsigned long addr);
void (*local_flush_data_cache_page)(void * addr);
void (*flush_data_cache_page)(unsigned long addr);
void (*flush_icache_all)(void);

EXPORT_SYMBOL_GPL(local_flush_data_cache_page);
EXPORT_SYMBOL(flush_data_cache_page);
EXPORT_SYMBOL(flush_icache_all);

Regardless, I'm happy to stick it in the middle of the function
pointers if that's preferred.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] mips: export icache_flush_range

[Sergei Shtylyov]

On 03/23/2014 12:05 AM, Kees Cook wrote:

>>>>> The lkdtm module performs tests against executable memory ranges, so
>>>>> it needs to flush the icache for proper behaviors. Other architectures
>>>>> already export this, so do the same for MIPS.

>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>> This is currently untested! I'm building a MIPS cross-compiler now...
>>>>> If someone can validate this fixes the build when lkdtm is a module,
>>>>> that would be appreciated. :)
>>>>> ---
>>>>>    arch/mips/mm/cache.c |    1 +
>>>>>    1 file changed, 1 insertion(+)

>>>>> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
>>>>> index fde7e56d13fe..b3f1df13d9f6 100644
>>>>> --- a/arch/mips/mm/cache.c
>>>>> +++ b/arch/mips/mm/cache.c
>>>>> @@ -38,6 +38,7 @@ void (*__flush_kernel_vmap_range)(unsigned long vaddr,
>>>>> int
>>>>> size);
>>>>>    void (*__invalidate_kernel_vmap_range)(unsigned long vaddr, int size);
>>>>>
>>>>>    EXPORT_SYMBOL_GPL(__flush_kernel_vmap_range);
>>>>> +EXPORT_SYMBOL_GPL(flush_icache_range);

>>>>      Have you run this thru scripts/checkpatch.pl? It would have told you
>>>> that
>>>> an export should immediately follow the corresponding function body,
>>>> AFAIK.

>>>      Hm, it doesn't now but definitely used to...

>>     Decided to check Documentation/CodingStyle, and it still codifies this:

>> In source files, separate functions with one blank line.  If the function is
>> exported, the EXPORT* macro for it should follow immediately after the
>> closing
>> function brace line.  E.g.:

>> int system_is_up(void)
>> {
>>          return system_state == SYSTEM_RUNNING;
>> }
>> EXPORT_SYMBOL(system_is_up);

> Yup, thanks. I know the style, but it seemed from the source file that
> the style was to declare the function pointers in bulk, and then
> explicitly export them in the next section, so I continued that style.
> For example:

> void (*flush_cache_sigtramp)(unsigned long addr);
> void (*local_flush_data_cache_page)(void * addr);
> void (*flush_data_cache_page)(unsigned long addr);
> void (*flush_icache_all)(void);

> EXPORT_SYMBOL_GPL(local_flush_data_cache_page);
> EXPORT_SYMBOL(flush_data_cache_page);
> EXPORT_SYMBOL(flush_icache_all);

> Regardless, I'm happy to stick it in the middle of the function
> pointers if that's preferred.

    Ah, that's a variable! I didn't realize that. Looking into checkpatch.pl, 
it should warn about variables as well though... but it doesn't.

> -Kees

WBR, Sergei