Re: [PATCH 33/48] perf dwarf-aux: Check allowed DWARF Ops
From: Namhyung Kim
Date: Wed Nov 08 2023 - 00:35:02 EST
On Tue, Nov 7, 2023 at 1:32 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> On Wed, 11 Oct 2023 20:50:56 -0700
> Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> > The DWARF location expression can be fairly complex and it'd be hard
> > to match it with the condition correctly. So let's be conservative
> > and only allow simple expressions. For now it just checks the first
> > operation in the list. The following operations looks ok:
> >
> > * DW_OP_stack_value
> > * DW_OP_deref_size
> > * DW_OP_deref
> > * DW_OP_piece
> >
> > To refuse complex (and unsupported) location expressions, add
> > check_allowed_ops() to compare the rest of the list. It seems earlier
> > result contained those unsupported expressions. For example, I found
> > some local struct variable is placed like below.
> >
> > <2><43d1517>: Abbrev Number: 62 (DW_TAG_variable)
> > <43d1518> DW_AT_location : 15 byte block: 91 50 93 8 91 78 93 4 93 84 8 91 68 93 4
> > (DW_OP_fbreg: -48; DW_OP_piece: 8;
> > DW_OP_fbreg: -8; DW_OP_piece: 4;
> > DW_OP_piece: 1028;
> > DW_OP_fbreg: -24; DW_OP_piece: 4)
> >
> > Another example is something like this.
> >
> > 0057c8be ffffffffffffffff ffffffff812109f0 (base address)
> > 0057c8ce ffffffff812112b5 ffffffff812112c8 (DW_OP_breg3 (rbx): 0;
> > DW_OP_constu: 18446744073709551612;
> > DW_OP_and;
> > DW_OP_stack_value)
> >
> > It should refuse them. After the change, the stat shows:
> >
> > Annotate data type stats:
> > total 294, ok 158 (53.7%), bad 136 (46.3%)
> > -----------------------------------------------------------
> > 30 : no_sym
> > 32 : no_mem_ops
> > 53 : no_var
> > 14 : no_typeinfo
> > 7 : bad_offset
> >
>
> The code itself looks good to me.
>
> Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
Thanks!
>
> If this fixes the previous patch in the same series (this seems a fix for the
> main usecase), please make it to a single patch.
Well.. I think it's a fix and also a quality issue. And I'd like to
have this separately to document and confirm which operations
are safe or acceptable. I listed 4 operations here based on my
local tests but I may miss something.
Thanks,
Namhyung
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> > tools/perf/util/dwarf-aux.c | 44 +++++++++++++++++++++++++++++++++----
> > 1 file changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index 7f3822d08ab7..093d7e82b333 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -1305,6 +1305,34 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
> > return true;
> > }
> >
> > +static bool check_allowed_ops(Dwarf_Op *ops, size_t nops)
> > +{
> > + /* The first op is checked separately */
> > + ops++;
> > + nops--;
> > +
> > + /*
> > + * It needs to make sure if the location expression matches to the given
> > + * register and offset exactly. Thus it rejects any complex expressions
> > + * and only allows a few of selected operators that doesn't change the
> > + * location.
> > + */
> > + while (nops) {
> > + switch (ops->atom) {
> > + case DW_OP_stack_value:
> > + case DW_OP_deref_size:
> > + case DW_OP_deref:
> > + case DW_OP_piece:
> > + break;
> > + default:
> > + return false;
> > + }
> > + ops++;
> > + nops--;
> > + }
> > + return true;
> > +}
> > +
> > /* Only checks direct child DIEs in the given scope. */
> > static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> > {
> > @@ -1332,25 +1360,31 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> > /* Local variables accessed using frame base register */
> > if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
> > data->offset >= (int)ops->number &&
> > + check_allowed_ops(ops, nops) &&
> > match_var_offset(die_mem, data, data->offset, ops->number))
> > return DIE_FIND_CB_END;
> >
> > /* Only match with a simple case */
> > if (data->reg < DWARF_OP_DIRECT_REGS) {
> > - if (ops->atom == (DW_OP_reg0 + data->reg) && nops == 1)
> > + /* pointer variables saved in a register 0 to 31 */
> > + if (ops->atom == (DW_OP_reg0 + data->reg) &&
> > + check_allowed_ops(ops, nops))
> > return DIE_FIND_CB_END;
> >
> > /* Local variables accessed by a register + offset */
> > if (ops->atom == (DW_OP_breg0 + data->reg) &&
> > + check_allowed_ops(ops, nops) &&
> > match_var_offset(die_mem, data, data->offset, ops->number))
> > return DIE_FIND_CB_END;
> > } else {
> > + /* pointer variables saved in a register 32 or above */
> > if (ops->atom == DW_OP_regx && ops->number == data->reg &&
> > - nops == 1)
> > + check_allowed_ops(ops, nops))
> > return DIE_FIND_CB_END;
> >
> > /* Local variables accessed by a register + offset */
> > if (ops->atom == DW_OP_bregx && data->reg == ops->number &&
> > + check_allowed_ops(ops, nops) &&
> > match_var_offset(die_mem, data, data->offset, ops->number2))
> > return DIE_FIND_CB_END;
> > }
> > @@ -1412,7 +1446,8 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
> > if (data->addr < ops->number)
> > continue;
> >
> > - if (match_var_offset(die_mem, data, data->addr, ops->number))
> > + if (check_allowed_ops(ops, nops) &&
> > + match_var_offset(die_mem, data, data->addr, ops->number))
> > return DIE_FIND_CB_END;
> > }
> > return DIE_FIND_CB_SIBLING;
> > @@ -1501,7 +1536,8 @@ int die_get_cfa(Dwarf *dwarf, u64 pc, int *preg, int *poffset)
> > return -1;
> >
> > if (!dwarf_cfi_addrframe(cfi, pc, &frame) &&
> > - !dwarf_frame_cfa(frame, &ops, &nops) && nops == 1) {
> > + !dwarf_frame_cfa(frame, &ops, &nops) &&
> > + check_allowed_ops(ops, nops)) {
> > *preg = reg_from_dwarf_op(ops);
> > *poffset = offset_from_dwarf_op(ops);
> > return 0;
> > --
> > 2.42.0.655.g421f12c284-goog
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>