PING: RFR: 8236489: Java heap file on daxfs should be more secure

[David Holmes]

On 6/01/2020 6:02 pm, Yasumasa Suenaga wrote:
> Hi David,
> 
> Thanks for your comment!
> 
> On 2020/01/06 16:30, David Holmes wrote:
>> Correction ...
>>
>> On 6/01/2020 5:22 pm, David Holmes wrote:
>>> Hi Yasumasa,
>>>
>>> On 6/01/2020 2:13 pm, Yasumasa Suenaga wrote:
>>>> Please review. I need one more reviewer to push.
>>>>
>>>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8236489
>>>>>>    webrev: 
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8236489/webrev.00/
>>>
>>> So you are expecting that if you build on a newer linux and run on an 
>>> older pre-3.11 version then this code:
>>>
>>>   194   fd = os::open(dir, O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);
>>>
>>> will set fd to -1. But I can't see any documented guarantee that will 
>>> be the case. It will depend how extensively open examines the flags 
>>> argument for unexpected values. If the flag is simply ignored then 
>>> you will successfully open thew directory - no?
>>
>> Presumably no - per the below. The intent is that O_TMPFILE will be 
>> defined in such a way that its use will cause an error on older kernels.
> 
> If it executes on older kernel, open(2) returns error with EISDIR.
> It is documented in manpage of open(2).
> 
>    http://man7.org/linux/man-pages/man2/open.2.html

Ah! Sorry I missed that when reading the manpage.

> 
>>> This also looks suspect:
>>>
>>>    67 #ifndef O_TMPFILE
>>>    68 #ifdef __O_TMPFILE
>>>    69 #define O_TMPFILE __O_TMPFILE
>>>    70 #endif
>>>    71 #endif
>>>
>>> when would we have __O_TMPFILE but not O_TMPFILE? 
>>
>> This question remains.
>>
>>> The definition on Linux itself is:
>>>
>>> #ifndef __O_TMPFILE
>>> #define __O_TMPFILE     020000000
>>> #endif
>>>
>>> /* a horrid kludge trying to make sure that this will fail on old 
>>> kernels */
>>> #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
>>
>> This was in asm-generic/fcntl.h but I think I should have been looking 
>> at bits/fcntl-linux.h which does the above in a cleaner way:
>>
>> #ifndef __O_TMPFILE
>> # define __O_TMPFILE   (020000000 | __O_DIRECTORY)
>> #endif
> 
> __O_TMPFILE seems to be added as an alternative of O_TMPFILE [1], so I 
> think we can use `#define O_TMPFILE __O_TMPFILE`.
> In addition, bits/fcntl-linux.h has similar code [2].

If __O_TMPFILE exists but not O_TMPFILE it means there is kernel support 
but no glibc support. So to my reading of [1] that means that the 
necessary code in open would not be present. It seems better to me to 
only try to handle O_TMPFILE when there is full glibc and kernel support.

Thanks,
David

> 
> Thanks,
> 
> Yasumasa
> 
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1471405
> [2] 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/bits/fcntl-linux.h;h=07a889d6831d2ec468f9dd5a2ba74fe03897e29c;hb=HEAD#l151 
> 
> 
> 
>> David
>>
>>> I think if we don't have O_TMPFILE that should be the end of it.
>>>
>>> ( I also don't like contaminating os_posix.cpp with non-POSIX 
>>> functionality. :( )
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2020/01/03 10:53, Yasumasa Suenaga wrote:
>>>>> PING: Could you review it?
>>>>>
>>>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8236489
>>>>>>    webrev: 
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8236489/webrev.00/
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2019/12/26 11:50, Yasumasa Suenaga wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I filed this to JBS. Could you review?
>>>>>>
>>>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8236489
>>>>>>    webrev: 
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8236489/webrev.00/
>>>>>>
>>>>>> It has passed all tests on submit repo. 
>>>>>> (mach5-one-ysuenaga-JDK-8236489-20191226-0145-7795073)
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2019/12/19 14:58, Yasumasa Suenaga wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> HotSpot allocates Java heap on daxfs if we pass -XX:AllocateHeapAt.
>>>>>>> It performs open(2) and unlink(2) on daxfs, and it is used via 
>>>>>>> mmap'ed address.
>>>>>>>
>>>>>>> mmap(2) would be called with MAP_SHARED, and it is not atomically 
>>>>>>> between open(2) and unlink(2). If malicious user open Java heap 
>>>>>>> file before unlink(2), it might be exposed.
>>>>>>>
>>>>>>> So I think we can use open(2) with O_TMPFILE instead of 
>>>>>>> mkstemp(3) as below.
>>>>>>>
>>>>>>>    http://cr.openjdk.java.net/~ysuenaga/dax/
>>>>>>>
>>>>>>> O_TMPFILE would create inode on filesystem, and it cannot be 
>>>>>>> accessed from out-of-process.
>>>>>>> However it cannot be provided in older Linux kernel. So I keep 
>>>>>>> current code as fall back.
>>>>>>>
>>>>>>>    http://man7.org/linux/man-pages/man2/open.2.html
>>>>>>>
>>>>>>> What do you think about it? or someone is working for it?
>>>>>>> If it is ok, I will file it to JBS and will send review request.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> P.S.
>>>>>>>    I tried to use MAP_PRIVATE for it, but it was slower than 
>>>>>>> MAP_SHARED.