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

[Yasumasa Suenaga]

Hi David,

O_TMPFILE will be defined if _GNU_SOURCE is defined.
(in fcntl-linux.h, it uses as __USE_GNU)

HotSpot sources are built with -D_GNU_SOURCE, so we need not to consider __O_TMPFILE.

So I uploaded new webrev.
It works fine on our machine.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8236489/webrev.01/


Thanks,

Yasumasa


On 2020/01/06 20:28, David Holmes wrote:
> 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.