RFR: Out-of-bounds access in cpu_family_description()

[David Holmes]

Thanks Vladimir!

David

On 13/04/2019 1:36 am, Vladimir Kozlov wrote:
> Looks good.
> 
> Thanks,
> Vladimir
> 
> On 4/12/19 12:50 AM, David Holmes wrote:
>> Hi Fanjinke,
>>
>> I have filed:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8222387
>>
>> On 12/04/2019 4:51 pm, Jinke Fan wrote:
>>> On 2019/4/12 14:20, David Holmes wrote:
>>>> Hi,
>>>>
>>>> Was there a reason you had to move the existing arrays before 
>>>> extending the amd one with the missing values?
>>> Yes, because the addition uses sizeof to control the boundary access.
>>> If the arrays's defined were behind the sizeof,it causes a 
>>> compilation error: "error: invalid application of 'sizeof’ to 
>>> incomplete type"
>>
>> Okay I found that a little crude so I went for a slightly nicer approach:
>>
>> http://cr.openjdk.java.net/~dholmes/8222387/webrev/
>>
>> Hope you don't mind.
>>
>> Thanks,
>> David
>>
>>> Best Regards!
>>> Fanjinke
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 12/04/2019 3:47 pm, Jinke Fan wrote:
>>>>> Hi David,
>>>>>      In VM_Version_Ext::cpu_family_description has out-of-bounds
>>>>> access on AMD 17h (EPYC) processor.
>>>>>
>>>>> const char* VM_Version_Ext::cpu_family_description(void) {
>>>>>
>>>>> On AMD 17h (EPYC) processor extended_cpu_family() will return 23,
>>>>> but array _family_id_amd only has 17 members.
>>>>>
>>>>>    int cpu_family_id = extended_cpu_family();
>>>>>    if (is_amd()) {
>>>>>      return _family_id_amd[cpu_family_id];
>>>>>    }
>>>>> ...
>>>>> }
>>>>>
>>>>> Result of testcase TestCPUInformation.java on AMD Zen:
>>>>> ----------System.out:(15/1615)----------
>>>>> ...
>>>>> Family: 386 (0x17), Model: <unknown> (0x1), Stepping: 0x1
>>>>> Ext. family: 0x8, Ext. model: 0x0, Type: 0x0, Signature: 0x00800f11
>>>>> ...
>>>>> }
>>>>>
>>>>> The “386” string is incorrectly and comes from Illegal access.
>>>>>
>>>>> The patch is based on the original repository:
>>>>> hg.openjdk.java.net/jdk/jdk
>>>>>
>>>>> changeset:   54520:f48312257bc6
>>>>> tag:         tip
>>>>> user:        vromero
>>>>> date:        Thu Apr 11 22:56:11 2019 -0400
>>>>> summary:     8222151: refactoring: enhancements to 
>>>>> java.lang.Class::methodToString and java.lang.Class::getTypeName
>>>>>
>>>>> *Patch
>>>>> The out of hg diff -g:
>>>>> diff --git a/src/hotspot/cpu/x86/vm_version_ext_x86.cpp 
>>>>> b/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>>> --- a/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>>> +++ b/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>>> @@ -262,6 +262,52 @@
>>>>>   int VM_Version_Ext::_no_of_cores = 0;
>>>>>   int VM_Version_Ext::_no_of_packages = 0;
>>>>>
>>>>> +const char* const VM_Version_Ext::_family_id_intel[] = {
>>>>> +  "8086/8088",
>>>>> +  "",
>>>>> +  "286",
>>>>> +  "386",
>>>>> +  "486",
>>>>> +  "Pentium",
>>>>> +  "Pentium Pro",   //or Pentium-M/Woodcrest depeding on model
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "Pentium 4"
>>>>> +};
>>>>> +
>>>>> +const char* const VM_Version_Ext::_family_id_amd[] = {
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "5x86",
>>>>> +  "K5/K6",
>>>>> +  "Athlon/AthlonXP",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "Opteron/Athlon64",
>>>>> +  "Opteron QC/Phenom",  // Barcelona et.al.
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "",
>>>>> +  "Zen"
>>>>> +};
>>>>> +
>>>>>   void VM_Version_Ext::initialize(void) {
>>>>>     ResourceMark rm;
>>>>>
>>>>> @@ -401,15 +447,19 @@
>>>>>   }
>>>>>
>>>>>   const char* VM_Version_Ext::cpu_family_description(void) {
>>>>> -  int cpu_family_id = extended_cpu_family();
>>>>> +  uint32_t cpu_family_id = extended_cpu_family();
>>>>>     if (is_amd()) {
>>>>> -    return _family_id_amd[cpu_family_id];
>>>>> +    if (cpu_family_id < 
>>>>> sizeof(_family_id_amd)/sizeof(_family_id_amd[0])) {
>>>>> +      return _family_id_amd[cpu_family_id];
>>>>> +    }
>>>>>     }
>>>>>     if (is_intel()) {
>>>>>       if (cpu_family_id == CPU_FAMILY_PENTIUMPRO) {
>>>>>         return cpu_model_description();
>>>>>       }
>>>>> -    return _family_id_intel[cpu_family_id];
>>>>> +    if (cpu_family_id < 
>>>>> sizeof(_family_id_intel)/sizeof(_family_id_intel[0])) {
>>>>> +      return _family_id_intel[cpu_family_id];
>>>>> +    }
>>>>>     }
>>>>>     if (is_hygon()) {
>>>>>       return "Dhyana";
>>>>> @@ -705,44 +755,6 @@
>>>>>     return _max_qualified_cpu_frequency;
>>>>>   }
>>>>>
>>>>> -const char* const VM_Version_Ext::_family_id_intel[] = {
>>>>> -  "8086/8088",
>>>>> -  "",
>>>>> -  "286",
>>>>> -  "386",
>>>>> -  "486",
>>>>> -  "Pentium",
>>>>> -  "Pentium Pro",   //or Pentium-M/Woodcrest depeding on model
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "Pentium 4"
>>>>> -};
>>>>> -
>>>>> -const char* const VM_Version_Ext::_family_id_amd[] = {
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "5x86",
>>>>> -  "K5/K6",
>>>>> -  "Athlon/AthlonXP",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "",
>>>>> -  "Opteron/Athlon64",
>>>>> -  "Opteron QC/Phenom"  // Barcelona et.al.
>>>>> -};
>>>>>   // Partially from Intel 64 and IA-32 Architecture Software 
>>>>> Developer's Manual,
>>>>>   // September 2013, Vol 3C Table 35-1
>>>>>   const char* const VM_Version_Ext::_model_id_pentium_pro[] = {
>>>>>
>>>>> *Test:
>>>>> After patched,result of testcase TestCPUInformation.java on AMD Zen:
>>>>> ----------System.out:(15/1615)----------
>>>>> Event: jdk.CPUInformation {
>>>>>    ...
>>>>> Family: Zen (0x17), Model: <unknown> (0x1), Stepping: 0x1
>>>>> Ext. family: 0x8, Ext. model: 0x0, Type: 0x0, Signature: 0x00800f11
>>>>> Features: ebx: 0x4f400800, ecx: 0x7ed8320b, edx: 0x178bfbff
>>>>>    ...
>>>>> }
>>>>>
>>>>> Is there anything incorrectly?
>>>>> Please let me know your comments.
>>>>>
>>>>> Best Regards！
>>>>> Fanjinke
>>>>>
>>>>
>>>