netdev - Re: [PATCH v3] tracing/net_sched: NULL pointer dereference in perf_trace_qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Tue, 25 Jun 2024 01:10:14 +0900
From: Yunseong Kim <yskelg@...il.com>
To: Pedro Tammela <pctammela@...atatu.com>
Cc: netdev@...r.kernel.org, stable@...r.kernel.org,
 Jakub Kicinski <kuba@...nel.org>, Steven Rostedt <rostedt@...dmis.org>,
 Masami Hiramatsu <mhiramat@...nel.org>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Takashi Iwai <tiwai@...e.de>, "David S. Miller" <davem@...emloft.net>,
 Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Jamal Hadi Salim
 <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
 Jiri Pirko <jiri@...nulli.us>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Taehee Yoo <ap420073@...il.com>,
 Austin Kim <austindh.kim@...il.com>, shjy180909@...il.com,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 ppbuk5246@...il.com, Yeoreum Yun <yeoreum.yun@....com>
Subject: Re: [PATCH v3] tracing/net_sched: NULL pointer dereference in
 perf_trace_qdisc_reset()

Hi Pedro,

On 6/25/24 12:55 오전, Pedro Tammela wrote:
> On 24/06/2024 12:43, Yunseong Kim wrote:
>> Hi Pedro,
>>
>> On 6/25/24 12:12 오전, Pedro Tammela wrote:
>>> On 22/06/2024 01:57, yskelg@...il.com wrote:
>>>> From: Yunseong Kim <yskelg@...il.com>
>>>>
>>>> In the TRACE_EVENT(qdisc_reset) NULL dereference occurred from
>>>>
>>>>    qdisc->dev_queue->dev <NULL> ->name
>>>>
>>>> [ 5301.595872] KASAN: null-ptr-deref in range
>>>> [0x0000000000000130-0x0000000000000137]
>>>> [ 5301.595877] Mem abort info:
>>>> [ 5301.595881]   ESR = 0x0000000096000006
>>>> [ 5301.595885]   EC = 0x25: DABT (current EL), IL = 32 bits
>>>> [ 5301.595889]   SET = 0, FnV = 0
>>>> [ 5301.595893]   EA = 0, S1PTW = 0
>>>> [ 5301.595896]   FSC = 0x06: level 2 translation fault
>>>> [ 5301.595900] Data abort info:
>>>> [ 5301.595903]   ISV = 0, ISS = 0x00000006, ISS2 = 0x00000000
>>>> [ 5301.595907]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>>>> [ 5301.595911]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>>>> [ 5301.595915] [dfff800000000026] address between user and kernel
>>>> address ranges
>>>> [ 5301.595971] Internal error: Oops: 0000000096000006 [#1] SMP
>>>> Link:
>>>> https://lore.kernel.org/lkml/20240229143432.273b4871@gandalf.local.home/t/
>>>> Fixes: 51270d573a8d ("tracing/net_sched: Fix tracepoints that save
>>>> qdisc_dev() as a string")
>>>> Cc: netdev@...r.kernel.org
>>>> Cc: stable@...r.kernel.org # +v6.7.10, +v6.8
>>>> Signed-off-by: Yunseong Kim <yskelg@...il.com>
>>>> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
>>>> ---
>>>>    include/trace/events/qdisc.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/trace/events/qdisc.h
>>>> b/include/trace/events/qdisc.h
>>>> index f1b5e816e7e5..170b51fbe47a 100644
>>>> --- a/include/trace/events/qdisc.h
>>>> +++ b/include/trace/events/qdisc.h
>>>> @@ -81,7 +81,7 @@ TRACE_EVENT(qdisc_reset,
>>>>        TP_ARGS(q),
>>>>          TP_STRUCT__entry(
>>>> -        __string(    dev,        qdisc_dev(q)->name    )
>>>> +        __string(dev, qdisc_dev(q) ? qdisc_dev(q)->name :
>>>> "noop_queue")
>>>>            __string(    kind,        q->ops->id        )
>>>>            __field(    u32,        parent            )
>>>>            __field(    u32,        handle            )
>>>
>>> You missed the __assign_str portion (see below). Also let's just say
>>> "(null)" as it's the correct device name. "noop_queue" could be
>>> misleading.
>>
>> Thanks for the code review Pedro, I agree your advice.
>>
>>> diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h
>>> index 1f4258308b96..f54e0b4dbcf4 100644
>>> --- a/include/trace/events/qdisc.h
>>> +++ b/include/trace/events/qdisc.h
>>> @@ -81,14 +81,14 @@ TRACE_EVENT(qdisc_reset,
>>>          TP_ARGS(q),
>>>
>>>          TP_STRUCT__entry(
>>> -               __string(       dev,           
>>> qdisc_dev(q)->name      )
>>> +               __string(       dev,            qdisc_dev(q) ?
>>> qdisc_dev(q)->name : "(null)"    )
>>>                  __string(       kind,          
>>> q->ops->id              )
>>>                  __field(        u32,           
>>> parent                  )
>>>                  __field(        u32,           
>>> handle                  )
>>>          ),
>>
>> It looks better to align the name with the current convention.
>>
>> Link:
>> https://lore.kernel.org/linux-trace-kernel/20240222211442.634192653@goodmis.org/
>>
>>>          TP_fast_assign(
>>> -               __assign_str(dev, qdisc_dev(q)->name);
>>> +               __assign_str(dev, qdisc_dev(q) ? qdisc_dev(q)->name :
>>> "(null)");
>>>                  __assign_str(kind, q->ops->id);
>>>                  __entry->parent = q->parent;
>>>                  __entry->handle = q->handle;
>>>
>>>
>>
>> The second part you mentioned, Steve recently worked on it and changed
>> it.
>>
>> Link:
>> https://lore.kernel.org/linux-trace-kernel/20240516133454.681ba6a0@rorschach.local.home/
> 
> Oh!

Thanks for the double check, Pedro.

>> If it hadn't, I don't think I would have been able to prevent the panic
>> by just applying my patch.
> 
> But you must be careful with the backports.
> 
> In any case, perhaps send another patch to net-next updating the new
> conventions there and use the 'old convention' for the bug fix?

Right, I agree, I'll send a patch for the next version.

Warm regards,
Yunseong Kim