On 11/8/17 6:47 AM, Y Song wrote:
> On Tue, Nov 7, 2017 at 1:39 PM, Alexei Starovoitov <[email protected]> wrote:
>> On 11/8/17 6:14 AM, Y Song wrote:
>>>
>>> On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
>>> <[email protected]> wrote:
>>>>
>>>> Alexei Starovoitov wrote:
>>>>>
>>>>>
>>>>> On 11/7/17 12:55 AM, Naveen N. Rao wrote:
>>>>>>>
>>>>>>>
>>>>>>> I thought such struct shouldn't change layout.
>>>>>>> If it is we need to fix include/linux/compiler-clang.h to do that
>>>>>>> anon struct as well.
>>>>>>
>>>>>>
>>>>>>
>>>>>> We considered that, but it looked to be very dependent on the version
>>>>>> of
>>>>>> gcc used to build the kernel. But, this may be a simpler approach for
>>>>>> the shorter term.
>>>>>>
>>>>>
>>>>> why it would depend on version of gcc?
>>>>
>>>>
>>>>
>>>> From what I can see, randomized_struct_fields_start is defined only for
>>>> gcc
>>>>>
>>>>> = 4.6. For older versions, it does not get mapped to an anonymous
>>>>
>>>> structure. We may not care for older gcc versions, but..
>>>>
>>>> The other issue was that __randomize_layout maps to __designated_init
>>>> when
>>>> randstruct plugin is not enabled, which is in turn an attribute on gcc >=
>>>> v5.1, but not otherwise.
>>>>
>>>>> We just need this, no?
>>>>>
>>>>> diff --git a/include/linux/compiler-clang.h
>>>>> b/include/linux/compiler-clang.h
>>>>> index de179993e039..4e29ab6187cb 100644
>>>>> --- a/include/linux/compiler-clang.h
>>>>> +++ b/include/linux/compiler-clang.h
>>>>> @@ -15,3 +15,6 @@
>>>>> * with any version that can compile the kernel
>>>>> */
>>>>> #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
>>>>> __COUNTER__)
>>>>> +
>>>>> +#define randomized_struct_fields_start struct {
>>>>> +#define randomized_struct_fields_end };
>>>>>
>>>>> since offsets are mandated by C standard.
>>>>
>>>>
>>>>
>>>> Yes, this is what we're testing with and is probably sufficient for our
>>>> purposes.
>>>
>>>
>>> Just tested this with bcc. bcc actually complains. the rewriter
>>> is not able to rewrite prev->pid where prev is "struct task_struct *prev".
>>> I will change bcc rewriter to see whether the field value is correct or
>>> not.
>>>
>>> Not sure my understanding is correct or not, but I am afraid that
>>> the above approach for clang compiler change may not work.
>>> If clang calculates the field offset based on header file, the offset
>>> may not be the same as kernel one....
>>
>>
>> why is that?
>> When randomization is off both gcc and clang must generate the same
>> offsets, since it's C standard.
>
> The patch changed compiler-clang.h, so gcc still do randomization.
gcc_plugins are off by default and randomization will not be
turned on for any sane distro or datacenter that cares about
performance and stability.
So imo above compiler-clang.h patch together with bcc fix would
be enough.
From 1583467998156655033@xxx Wed Nov 08 03:46:00 +0000 2017
X-GM-THRID: 1583027203607239623
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, Nov 7, 2017 at 2:04 PM, Alexei Starovoitov <[email protected]> wrote:
> On 11/8/17 6:47 AM, Y Song wrote:
>>
>> On Tue, Nov 7, 2017 at 1:39 PM, Alexei Starovoitov <[email protected]> wrote:
>>>
>>> On 11/8/17 6:14 AM, Y Song wrote:
>>>>
>>>>
>>>> On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
>>>> <[email protected]> wrote:
>>>>>
>>>>>
>>>>> Alexei Starovoitov wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/7/17 12:55 AM, Naveen N. Rao wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I thought such struct shouldn't change layout.
>>>>>>>> If it is we need to fix include/linux/compiler-clang.h to do that
>>>>>>>> anon struct as well.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> We considered that, but it looked to be very dependent on the version
>>>>>>> of
>>>>>>> gcc used to build the kernel. But, this may be a simpler approach for
>>>>>>> the shorter term.
>>>>>>>
>>>>>>
>>>>>> why it would depend on version of gcc?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> From what I can see, randomized_struct_fields_start is defined only for
>>>>> gcc
>>>>>>
>>>>>>
>>>>>> = 4.6. For older versions, it does not get mapped to an anonymous
>>>>>
>>>>>
>>>>> structure. We may not care for older gcc versions, but..
>>>>>
>>>>> The other issue was that __randomize_layout maps to __designated_init
>>>>> when
>>>>> randstruct plugin is not enabled, which is in turn an attribute on gcc
>>>>> >=
>>>>> v5.1, but not otherwise.
>>>>>
>>>>>> We just need this, no?
>>>>>>
>>>>>> diff --git a/include/linux/compiler-clang.h
>>>>>> b/include/linux/compiler-clang.h
>>>>>> index de179993e039..4e29ab6187cb 100644
>>>>>> --- a/include/linux/compiler-clang.h
>>>>>> +++ b/include/linux/compiler-clang.h
>>>>>> @@ -15,3 +15,6 @@
>>>>>> * with any version that can compile the kernel
>>>>>> */
>>>>>> #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
>>>>>> __COUNTER__)
>>>>>> +
>>>>>> +#define randomized_struct_fields_start struct {
>>>>>> +#define randomized_struct_fields_end };
>>>>>>
>>>>>> since offsets are mandated by C standard.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Yes, this is what we're testing with and is probably sufficient for our
>>>>> purposes.
>>>>
>>>>
>>>>
>>>> Just tested this with bcc. bcc actually complains. the rewriter
>>>> is not able to rewrite prev->pid where prev is "struct task_struct
>>>> *prev".
>>>> I will change bcc rewriter to see whether the field value is correct or
>>>> not.
>>>>
>>>> Not sure my understanding is correct or not, but I am afraid that
>>>> the above approach for clang compiler change may not work.
>>>> If clang calculates the field offset based on header file, the offset
>>>> may not be the same as kernel one....
>>>
>>>
>>>
>>> why is that?
>>> When randomization is off both gcc and clang must generate the same
>>> offsets, since it's C standard.
>>
>>
>> The patch changed compiler-clang.h, so gcc still do randomization.
>
>
> gcc_plugins are off by default and randomization will not be
> turned on for any sane distro or datacenter that cares about
> performance and stability.
> So imo above compiler-clang.h patch together with bcc fix would
> be enough.
Agree that short time the suggested fix should be enough.
Long time, disto could become "insane" someday :-)
From 1583464915287567078@xxx Wed Nov 08 02:57:00 +0000 2017
X-GM-THRID: 1583027203607239623
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread