From: Xu Kuohai <xukuohai@huawei.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: <bpf@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-kselftest@vger.kernel.org>, <netdev@vger.kernel.org>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <martin.lau@linux.dev>,
Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>,
Shuah Khan <shuah@kernel.org>,
"David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Jesper Dangaard Brouer <hawk@kernel.org>,
Kumar Kartikeya Dwivedi <memxor@gmail.com>,
Alan Maguire <alan.maguire@oracle.com>,
Delyan Kratunov <delyank@fb.com>,
Lorenzo Bianconi <lorenzo@kernel.org>
Subject: Re: [PATCH bpf v3 2/6] libbpf: Fix memory leak in parse_usdt_arg()
Date: Tue, 11 Oct 2022 14:26:33 +0800 [thread overview]
Message-ID: <a49eed59-8e26-fa9c-c7a6-8cb4656b0d55@huawei.com> (raw)
In-Reply-To: <CAEf4BzbJ8LW1Q_hBc-eB25f=F+jdQ5aPucEv_oDNrbjB=GGR+g@mail.gmail.com>
On 10/11/2022 9:34 AM, Andrii Nakryiko wrote:
> On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>
>> In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
>> is allocated but not freed. Fix it.
>>
>> Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>> tools/lib/bpf/usdt.c | 59 +++++++++++++++++++++++++-------------------
>> 1 file changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
>> index e83b497c2245..f3b5be7415b5 100644
>> --- a/tools/lib/bpf/usdt.c
>> +++ b/tools/lib/bpf/usdt.c
>> @@ -1351,8 +1351,10 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>> char *reg_name = NULL;
>> int arg_sz, len, reg_off;
>> long off;
>> + int ret;
>>
>> - if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, ®_name, &off, &len) == 3) {
>> + ret = sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, ®_name, &off, &len);
>> + if (ret == 3) {
>> /* Memory dereference case, e.g., -4@[sp, 96] */
>> arg->arg_type = USDT_ARG_REG_DEREF;
>> arg->val_off = off;
>> @@ -1361,32 +1363,37 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>> if (reg_off < 0)
>> return reg_off;
>> arg->reg_off = reg_off;
>> - } else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, ®_name, &len) == 2) {
>> - /* Memory dereference case, e.g., -4@[sp] */
>> - arg->arg_type = USDT_ARG_REG_DEREF;
>> - arg->val_off = 0;
>> - reg_off = calc_pt_regs_off(reg_name);
>> - free(reg_name);
>> - if (reg_off < 0)
>> - return reg_off;
>> - arg->reg_off = reg_off;
>> - } else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
>> - /* Constant value case, e.g., 4@5 */
>> - arg->arg_type = USDT_ARG_CONST;
>> - arg->val_off = off;
>> - arg->reg_off = 0;
>> - } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, ®_name, &len) == 2) {
>> - /* Register read case, e.g., -8@x4 */
>> - arg->arg_type = USDT_ARG_REG;
>> - arg->val_off = 0;
>> - reg_off = calc_pt_regs_off(reg_name);
>> - free(reg_name);
>> - if (reg_off < 0)
>> - return reg_off;
>> - arg->reg_off = reg_off;
>> } else {
>> - pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
>> - return -EINVAL;
>> + if (ret == 2)
>> + free(reg_name);
>> +
>> + if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, ®_name, &len) == 2) {
>> + /* Memory dereference case, e.g., -4@[sp] */
>> + arg->arg_type = USDT_ARG_REG_DEREF;
>> + arg->val_off = 0;
>> + reg_off = calc_pt_regs_off(reg_name);
>> + free(reg_name);
>> + if (reg_off < 0)
>> + return reg_off;
>> + arg->reg_off = reg_off;
>> + } else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
>> + /* Constant value case, e.g., 4@5 */
>> + arg->arg_type = USDT_ARG_CONST;
>> + arg->val_off = off;
>> + arg->reg_off = 0;
>> + } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, ®_name, &len) == 2) {
>> + /* Register read case, e.g., -8@x4 */
>> + arg->arg_type = USDT_ARG_REG;
>> + arg->val_off = 0;
>> + reg_off = calc_pt_regs_off(reg_name);
>> + free(reg_name);
>> + if (reg_off < 0)
>> + return reg_off;
>> + arg->reg_off = reg_off;
>> + } else {
>> + pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
>> + return -EINVAL;
>> + }
>> }
>>
>
> I think all this is more complicated than it has to be. How big can
> register names be? Few characters? Let's get rid of %m[a-z0-9] and
> instead use fixed-max-length strings, e.g., %5s. And read register
> names into such local char buffers. It will simplify everything
> tremendously. Let's use 16-byte buffers and use %15s to match it?
> Would that be enough?
>
The valid register names accepted by calc_pt_regs_off() are x0~x31 and sp, so
16-byte buffer is enough. Since %15s matches all non-space characters, will use
%15[a-z0-9] to match it.
>> arg->arg_signed = arg_sz < 0;
>> --
>> 2.30.2
>>
> .
next prev parent reply other threads:[~2022-10-11 6:27 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-10 14:25 [PATCH bpf v3 0/6] Fix bugs found by ASAN when running selftests Xu Kuohai
2022-10-10 14:25 ` [PATCH bpf v3 1/6] libbpf: Fix use-after-free in btf_dump_name_dups Xu Kuohai
2022-10-11 1:32 ` Andrii Nakryiko
2022-10-11 6:25 ` Xu Kuohai
2022-10-10 14:25 ` [PATCH bpf v3 2/6] libbpf: Fix memory leak in parse_usdt_arg() Xu Kuohai
2022-10-11 1:34 ` Andrii Nakryiko
2022-10-11 6:26 ` Xu Kuohai [this message]
2022-10-10 14:25 ` [PATCH bpf v3 3/6] selftests/bpf: Fix memory leak caused by not destroying skeleton Xu Kuohai
2022-10-10 14:25 ` [PATCH bpf v3 4/6] selftest/bpf: Fix memory leak in kprobe_multi_test Xu Kuohai
2022-10-11 1:34 ` Andrii Nakryiko
2022-10-11 6:26 ` Xu Kuohai
2022-10-10 14:25 ` [PATCH bpf v3 5/6] selftests/bpf: Fix error failure of case test_xdp_adjust_tail_grow Xu Kuohai
2022-10-10 14:25 ` [PATCH bpf v3 6/6] selftest/bpf: Fix error usage of ASSERT_OK in xdp_adjust_tail.c Xu Kuohai
2022-10-11 1:37 ` [PATCH bpf v3 0/6] Fix bugs found by ASAN when running selftests Andrii Nakryiko
2022-10-11 6:30 ` Xu Kuohai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a49eed59-8e26-fa9c-c7a6-8cb4656b0d55@huawei.com \
--to=xukuohai@huawei.com \
--cc=alan.maguire@oracle.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=delyank@fb.com \
--cc=haoluo@google.com \
--cc=hawk@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=lorenzo@kernel.org \
--cc=martin.lau@linux.dev \
--cc=memxor@gmail.com \
--cc=mykolal@fb.com \
--cc=netdev@vger.kernel.org \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.