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

David Holmes david.holmes at oracle.com
Tue Jan 7 01:46:27 UTC 2020 

Thanks Yasumasa, no further comments from me.

David

On 7/01/2020 11:38 am, Yasumasa Suenaga wrote:
> Hi David,
> 
> On 2020/01/07 7:38, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 6/01/2020 11:45 pm, Yasumasa Suenaga wrote:
>>> 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/
>>
>> I'd be slightly more comfortable if we changed:
>>
>>   181 #ifdef O_TMPFILE
>>
>> to
>>
>>   #if defined(linux) && defined(O_TMPFILE)
>>
>> just in case some other OS defines O_TMPFILE.
> 
> I fixed it in new webrev:
> 
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8236489/webrev.02/
> 
> 
>> So from a code perspective this seems ok.
>>
>> Semantically I'm still a little vague on the details and how this is 
>> specific to daxfs. Where does the unlink occur that causes the 
>> atomicity issue between the open and unlink? IIUC mkstemp is creating 
>> a temporary named file that is visible in the filesystem, while 
>> O_TMPFILE is creating (in this usage) an unnamed temp file, which is 
>> not visible in the filesystem. Is that right?
> 
> Yes, mkstemp() would create regular file on filesystem, so we need to 
> unlink it immediately.
> OTOH O_TMPFILE would create unnamed temp file.
> 
> This issue is also appear on excepting daxfs (e.g. ext4).
> AFAIK -XX:AllocateHeapAt is introduced in JEP 316 for persistent memory 
> device.
> So I explained this issue as daxfs - sorry for confusing.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
>> Thanks,
>> David
>>
>>>
>>> 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.

More information about the hotspot-runtime-dev mailing list