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

Tue Jan 7 01:38:13 UTC 2020

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.