From 327303315367f15113c17c8525d8eb3bc0f66782 Mon Sep 17 00:00:00 2001 From: Jan Kratochvil Date: Wed, 10 Sep 2025 18:12:18 -0700 Subject: [PATCH] 8352637: Enhance bytecode verification Reviewed-by: fferrari, andrew Backport-of: 974f4da2e53ebde8b06224f4ba0b80aa74c5f434 --- .../src/share/vm/classfile/stackMapTable.cpp | 10 ++++- .../src/share/vm/classfile/stackMapTable.hpp | 2 +- hotspot/src/share/vm/classfile/verifier.cpp | 35 +++++++++-------- .../share/vm/interpreter/bytecodeStream.hpp | 19 ++++++++- jdk/src/share/native/common/check_code.c | 39 +++++++++++++------ 5 files changed, 74 insertions(+), 31 deletions(-) diff --git a/hotspot/src/share/vm/classfile/stackMapTable.cpp b/hotspot/src/share/vm/classfile/stackMapTable.cpp index 547dcf64b9..13606ae161 100644 --- a/hotspot/src/share/vm/classfile/stackMapTable.cpp +++ b/hotspot/src/share/vm/classfile/stackMapTable.cpp @@ -122,8 +122,16 @@ bool StackMapTable::match_stackmap( } void StackMapTable::check_jump_target( - StackMapFrame* frame, int32_t target, TRAPS) const { + StackMapFrame* frame, int bci, int offset, TRAPS) const { ErrorContext ctx; + // Jump targets must be within the method and the method size is limited. See JVMS 4.11 + int min_offset = -1 * max_method_code_size; + if (offset < min_offset || offset > max_method_code_size) { + frame->verifier()->verify_error(ErrorContext::bad_stackmap(bci, frame), + "Illegal target of jump or branch (bci %d + offset %d)", bci, offset); + return; + } + int target = bci + offset; bool match = match_stackmap( frame, target, true, false, &ctx, CHECK_VERIFY(frame->verifier())); if (!match || (target < 0 || target >= _code_length)) { diff --git a/hotspot/src/share/vm/classfile/stackMapTable.hpp b/hotspot/src/share/vm/classfile/stackMapTable.hpp index a36a7ba3cf..36fddb3988 100644 --- a/hotspot/src/share/vm/classfile/stackMapTable.hpp +++ b/hotspot/src/share/vm/classfile/stackMapTable.hpp @@ -86,7 +86,7 @@ class StackMapTable : public StackObj { // Check jump instructions. Make sure there are no uninitialized // instances on backward branch. - void check_jump_target(StackMapFrame* frame, int32_t target, TRAPS) const; + void check_jump_target(StackMapFrame* frame, int bci, int offset, TRAPS) const; // The following methods are only used inside this class. diff --git a/hotspot/src/share/vm/classfile/verifier.cpp b/hotspot/src/share/vm/classfile/verifier.cpp index 2dddd1fded..4662bf02a5 100644 --- a/hotspot/src/share/vm/classfile/verifier.cpp +++ b/hotspot/src/share/vm/classfile/verifier.cpp @@ -668,7 +668,6 @@ void ClassVerifier::verify_method(methodHandle m, TRAPS) { // Merge with the next instruction { u2 index; - int target; VerificationType type, type2; VerificationType atype; @@ -1480,9 +1479,8 @@ void ClassVerifier::verify_method(methodHandle m, TRAPS) { case Bytecodes::_ifle: current_frame.pop_stack( VerificationType::integer_type(), CHECK_VERIFY(this)); - target = bcs.dest(); stackmap_table.check_jump_target( - ¤t_frame, target, CHECK_VERIFY(this)); + ¤t_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this)); no_control_flow = false; break; case Bytecodes::_if_acmpeq : case Bytecodes::_if_acmpne : @@ -1493,19 +1491,16 @@ void ClassVerifier::verify_method(methodHandle m, TRAPS) { case Bytecodes::_ifnonnull : current_frame.pop_stack( VerificationType::reference_check(), CHECK_VERIFY(this)); - target = bcs.dest(); stackmap_table.check_jump_target - (¤t_frame, target, CHECK_VERIFY(this)); + (¤t_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this)); no_control_flow = false; break; case Bytecodes::_goto : - target = bcs.dest(); stackmap_table.check_jump_target( - ¤t_frame, target, CHECK_VERIFY(this)); + ¤t_frame, bcs.bci(), bcs.get_offset_s2(), CHECK_VERIFY(this)); no_control_flow = true; break; case Bytecodes::_goto_w : - target = bcs.dest_w(); stackmap_table.check_jump_target( - ¤t_frame, target, CHECK_VERIFY(this)); + ¤t_frame, bcs.bci(), bcs.get_offset_s4(), CHECK_VERIFY(this)); no_control_flow = true; break; case Bytecodes::_tableswitch : case Bytecodes::_lookupswitch : @@ -2107,15 +2102,14 @@ void ClassVerifier::verify_switch( } } } - int target = bci + default_offset; - stackmap_table->check_jump_target(current_frame, target, CHECK_VERIFY(this)); + stackmap_table->check_jump_target(current_frame, bci, default_offset, CHECK_VERIFY(this)); for (int i = 0; i < keys; i++) { // Because check_jump_target() may safepoint, the bytecode could have // moved, which means 'aligned_bcp' is no good and needs to be recalculated. aligned_bcp = (address)round_to((intptr_t)(bcs->bcp() + 1), jintSize); - target = bci + (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize); + int offset = (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize); stackmap_table->check_jump_target( - current_frame, target, CHECK_VERIFY(this)); + current_frame, bci, offset, CHECK_VERIFY(this)); } NOT_PRODUCT(aligned_bcp = NULL); // no longer valid at this point } @@ -2376,8 +2370,13 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) { break; case Bytecodes::_goto: - case Bytecodes::_goto_w: - target = (opcode == Bytecodes::_goto ? bcs.dest() : bcs.dest_w()); + case Bytecodes::_goto_w: { + int offset = (opcode == Bytecodes::_goto ? bcs.get_offset_s2() : bcs.get_offset_s4()); + int min_offset = -1 * max_method_code_size; + // Check offset for overflow + if (offset < min_offset || offset > max_method_code_size) return false; + + target = bci + offset; if (visited_branches->contains(bci)) { if (bci_stack->is_empty()) { if (handler_stack->is_empty()) { @@ -2398,6 +2397,7 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) { visited_branches->append(bci); } break; + } // Check that all switch alternatives end in 'athrow' bytecodes. Since it // is difficult to determine where each switch alternative ends, parse @@ -2434,7 +2434,10 @@ bool ClassVerifier::ends_in_athrow(u4 start_bc_offset) { // Push the switch alternatives onto the stack. for (int i = 0; i < keys; i++) { - u4 target = bci + (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize); + int min_offset = -1 * max_method_code_size; + int offset = (jint)Bytes::get_Java_u4(aligned_bcp+(3+i*delta)*jintSize); + if (offset < min_offset || offset > max_method_code_size) return false; + u4 target = bci + offset; if (target > code_length) return false; bci_stack->push(target); } diff --git a/hotspot/src/share/vm/interpreter/bytecodeStream.hpp b/hotspot/src/share/vm/interpreter/bytecodeStream.hpp index b814b88d5d..20f38950e1 100644 --- a/hotspot/src/share/vm/interpreter/bytecodeStream.hpp +++ b/hotspot/src/share/vm/interpreter/bytecodeStream.hpp @@ -121,8 +121,23 @@ class BaseBytecodeStream: StackObj { void set_next_bci(int bci) { assert(0 <= bci && bci <= method()->code_size(), "illegal bci"); _next_bci = bci; } // Bytecode-specific attributes - int dest() const { return bci() + bytecode().get_offset_s2(raw_code()); } - int dest_w() const { return bci() + bytecode().get_offset_s4(raw_code()); } + int get_offset_s2() const { return bytecode().get_offset_s2(raw_code()); } + int get_offset_s4() const { return bytecode().get_offset_s4(raw_code()); } + + // These methods are not safe to use before or during verification as they may + // have large offsets and cause overflows + int dest() const { + int min_offset = -1 * max_method_code_size; + int offset = bytecode().get_offset_s2(raw_code()); + guarantee(offset >= min_offset && offset <= max_method_code_size, "must be"); + return bci() + offset; + } + int dest_w() const { + int min_offset = -1 * max_method_code_size; + int offset = bytecode().get_offset_s4(raw_code()); + guarantee(offset >= min_offset && offset <= max_method_code_size, "must be"); + return bci() + offset; + } // One-byte indices. int get_index_u1() const { assert_raw_index_size(1); return *(jubyte*)(bcp()+1); } diff --git a/jdk/src/share/native/common/check_code.c b/jdk/src/share/native/common/check_code.c index 799b8c654c..0889c57f49 100644 --- a/jdk/src/share/native/common/check_code.c +++ b/jdk/src/share/native/common/check_code.c @@ -396,7 +396,8 @@ static jboolean is_superclass(context_type *, fullinfo_type); static void initialize_exception_table(context_type *); static int instruction_length(unsigned char *iptr, unsigned char *end); -static jboolean isLegalTarget(context_type *, int offset); +static jboolean isLegalOffset(context_type *, int bci, int offset); +static jboolean isLegalTarget(context_type *, int target); static void verify_constant_pool_type(context_type *, int, unsigned); static void initialize_dataflow(context_type *); @@ -1161,9 +1162,9 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset) case JVM_OPC_goto: { /* Set the ->operand to be the instruction number of the target. */ int jump = (((signed char)(code[offset+1])) << 8) + code[offset+2]; - int target = offset + jump; - if (!isLegalTarget(context, target)) + if (!isLegalOffset(context, offset, jump)) CCerror(context, "Illegal target of jump or branch"); + int target = offset + jump; this_idata->operand.i = code_data[target]; break; } @@ -1177,9 +1178,9 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset) int jump = (((signed char)(code[offset+1])) << 24) + (code[offset+2] << 16) + (code[offset+3] << 8) + (code[offset + 4]); - int target = offset + jump; - if (!isLegalTarget(context, target)) + if (!isLegalOffset(context, offset, jump)) CCerror(context, "Illegal target of jump or branch"); + int target = offset + jump; this_idata->operand.i = code_data[target]; break; } @@ -1218,13 +1219,16 @@ verify_opcode_operands(context_type *context, unsigned int inumber, int offset) } } saved_operand = NEW(int, keys + 2); - if (!isLegalTarget(context, offset + _ck_ntohl(lpc[0]))) + int jump = _ck_ntohl(lpc[0]); + if (!isLegalOffset(context, offset, jump)) CCerror(context, "Illegal default target in switch"); - saved_operand[keys + 1] = code_data[offset + _ck_ntohl(lpc[0])]; + int target = offset + jump; + saved_operand[keys + 1] = code_data[target]; for (k = keys, lptr = &lpc[3]; --k >= 0; lptr += delta) { - int target = offset + _ck_ntohl(lptr[0]); - if (!isLegalTarget(context, target)) + jump = _ck_ntohl(lptr[0]); + if (!isLegalOffset(context, offset, jump)) CCerror(context, "Illegal branch in tableswitch"); + target = offset + jump; saved_operand[k + 1] = code_data[target]; } saved_operand[0] = keys + 1; /* number of successors */ @@ -1746,11 +1750,24 @@ static int instruction_length(unsigned char *iptr, unsigned char *end) /* Given the target of a branch, make sure that it's a legal target. */ static jboolean -isLegalTarget(context_type *context, int offset) +isLegalTarget(context_type *context, int target) { int code_length = context->code_length; int *code_data = context->code_data; - return (offset >= 0 && offset < code_length && code_data[offset] >= 0); + return (target >= 0 && target < code_length && code_data[target] >= 0); +} + +/* Given a bci and offset, make sure the offset is valid and the target is legal */ +static jboolean +isLegalOffset(context_type *context, int bci, int offset) +{ + int code_length = context->code_length; + int *code_data = context->code_data; + int max_offset = 65535; // JVMS 4.11 + int min_offset = -65535; + if (offset < min_offset || offset > max_offset) return JNI_FALSE; + int target = bci + offset; + return (target >= 0 && target < code_length && code_data[target] >= 0); }