[v2] bpftool: Check map name length when map create

[PATCH bpf-next v2] bpftool: Check map name length when map create

Posted by Rong Tao 10 months, 1 week ago

From: Rong Tao <rongtao@cestc.cn>

The size of struct bpf_map::name is BPF_OBJ_NAME_LEN (16).

bpf(2) {
  map_create() {
    bpf_obj_name_cpy(map->name, attr->map_name, sizeof(attr->map_name));
  }
}

When specifying a map name using bpftool map create name, no error is
reported if the name length is greater than 15.

    $ sudo bpftool map create /sys/fs/bpf/12345678901234567890 \
        type array key 4 value 4 entries 5 name 12345678901234567890

Users will think that 12345678901234567890 is legal, but this name cannot
be used to index a map.

    $ sudo bpftool map show name 12345678901234567890
    Error: can't parse name

    $ sudo bpftool map show
    ...
    1249: array  name 123456789012345  flags 0x0
    	key 4B  value 4B  max_entries 5  memlock 304B

    $ sudo bpftool map show name 123456789012345
    1249: array  name 123456789012345  flags 0x0
    	key 4B  value 4B  max_entries 5  memlock 304B

The map name provided in the command line is truncated, but no error is
reported. This submission checks the length of the map name.

Signed-off-by: Rong Tao <rongtao@cestc.cn>
---
 tools/bpf/bpftool/map.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index ed4a9bd82931..bd5df44e2420 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1270,6 +1270,11 @@ static int do_create(int argc, char **argv)
 		} else if (is_prefix(*argv, "name")) {
 			NEXT_ARG();
 			map_name = GET_ARG();
+			if (strlen(map_name) > BPF_OBJ_NAME_LEN - 1) {
+				p_err("map name must be no longer than %d characters\n",
+				      BPF_OBJ_NAME_LEN - 1);
+				goto exit;
+			}
 		} else if (is_prefix(*argv, "key")) {
 			if (parse_u32_arg(&argc, &argv, &key_size,
 					  "key size"))
-- 
2.48.1

Re: [PATCH bpf-next v2] bpftool: Check map name length when map create

Posted by Quentin Monnet 10 months, 1 week ago

2025-02-11 18:38 UTC+0800 ~ Rong Tao <rtoax@foxmail.com>
> From: Rong Tao <rongtao@cestc.cn>
> 
> The size of struct bpf_map::name is BPF_OBJ_NAME_LEN (16).
> 
> bpf(2) {
>   map_create() {
>     bpf_obj_name_cpy(map->name, attr->map_name, sizeof(attr->map_name));
>   }
> }
> 
> When specifying a map name using bpftool map create name, no error is
> reported if the name length is greater than 15.
> 
>     $ sudo bpftool map create /sys/fs/bpf/12345678901234567890 \
>         type array key 4 value 4 entries 5 name 12345678901234567890
> 
> Users will think that 12345678901234567890 is legal, but this name cannot
> be used to index a map.
> 
>     $ sudo bpftool map show name 12345678901234567890
>     Error: can't parse name
> 
>     $ sudo bpftool map show
>     ...
>     1249: array  name 123456789012345  flags 0x0
>     	key 4B  value 4B  max_entries 5  memlock 304B
> 
>     $ sudo bpftool map show name 123456789012345
>     1249: array  name 123456789012345  flags 0x0
>     	key 4B  value 4B  max_entries 5  memlock 304B
> 
> The map name provided in the command line is truncated, but no error is
> reported. This submission checks the length of the map name.
> 
> Signed-off-by: Rong Tao <rongtao@cestc.cn>


Reviewed-by: Quentin Monnet <qmo@kernel.org>

Thank you!

Re: [PATCH bpf-next v2] bpftool: Check map name length when map create

Posted by Andrii Nakryiko 10 months, 1 week ago

On Tue, Feb 11, 2025 at 2:48 AM Quentin Monnet <qmo@kernel.org> wrote:
>
> 2025-02-11 18:38 UTC+0800 ~ Rong Tao <rtoax@foxmail.com>
> > From: Rong Tao <rongtao@cestc.cn>
> >
> > The size of struct bpf_map::name is BPF_OBJ_NAME_LEN (16).
> >
> > bpf(2) {
> >   map_create() {
> >     bpf_obj_name_cpy(map->name, attr->map_name, sizeof(attr->map_name));
> >   }
> > }
> >
> > When specifying a map name using bpftool map create name, no error is
> > reported if the name length is greater than 15.
> >
> >     $ sudo bpftool map create /sys/fs/bpf/12345678901234567890 \
> >         type array key 4 value 4 entries 5 name 12345678901234567890
> >
> > Users will think that 12345678901234567890 is legal, but this name cannot
> > be used to index a map.
> >
> >     $ sudo bpftool map show name 12345678901234567890
> >     Error: can't parse name
> >
> >     $ sudo bpftool map show
> >     ...
> >     1249: array  name 123456789012345  flags 0x0
> >       key 4B  value 4B  max_entries 5  memlock 304B
> >
> >     $ sudo bpftool map show name 123456789012345
> >     1249: array  name 123456789012345  flags 0x0
> >       key 4B  value 4B  max_entries 5  memlock 304B
> >
> > The map name provided in the command line is truncated, but no error is
> > reported. This submission checks the length of the map name.
> >
> > Signed-off-by: Rong Tao <rongtao@cestc.cn>
>
>
> Reviewed-by: Quentin Monnet <qmo@kernel.org>
>

Would it make sense to just warn but proceed with a truncated name?
libbpf truncates the name when creating a map, but preserves the
original name in BTF (and in memory, fetchable through
bpf_map__name()). So from the user's perspective that map is still
named "blah-blah-something-long", even if the kernel records just a
prefix of that.

Basically, instead of forcing users to count the first 15 characters,
warn, but do the right thing anyways?

> Thank you!

Re: [PATCH bpf-next v2] bpftool: Check map name length when map create

Posted by Rong Tao 10 months, 1 week ago

On 2/12/25 05:07, Andrii Nakryiko wrote:
> On Tue, Feb 11, 2025 at 2:48 AM Quentin Monnet <qmo@kernel.org> wrote:
>> 2025-02-11 18:38 UTC+0800 ~ Rong Tao <rtoax@foxmail.com>
>>> From: Rong Tao <rongtao@cestc.cn>
>>>
>>> The size of struct bpf_map::name is BPF_OBJ_NAME_LEN (16).
>>>
>>> bpf(2) {
>>>    map_create() {
>>>      bpf_obj_name_cpy(map->name, attr->map_name, sizeof(attr->map_name));
>>>    }
>>> }
>>>
>>> When specifying a map name using bpftool map create name, no error is
>>> reported if the name length is greater than 15.
>>>
>>>      $ sudo bpftool map create /sys/fs/bpf/12345678901234567890 \
>>>          type array key 4 value 4 entries 5 name 12345678901234567890
>>>
>>> Users will think that 12345678901234567890 is legal, but this name cannot
>>> be used to index a map.
>>>
>>>      $ sudo bpftool map show name 12345678901234567890
>>>      Error: can't parse name
>>>
>>>      $ sudo bpftool map show
>>>      ...
>>>      1249: array  name 123456789012345  flags 0x0
>>>        key 4B  value 4B  max_entries 5  memlock 304B
>>>
>>>      $ sudo bpftool map show name 123456789012345
>>>      1249: array  name 123456789012345  flags 0x0
>>>        key 4B  value 4B  max_entries 5  memlock 304B
>>>
>>> The map name provided in the command line is truncated, but no error is
>>> reported. This submission checks the length of the map name.
>>>
>>> Signed-off-by: Rong Tao <rongtao@cestc.cn>
>>
>> Reviewed-by: Quentin Monnet <qmo@kernel.org>
>>
> Would it make sense to just warn but proceed with a truncated name?
> libbpf truncates the name when creating a map, but preserves the
> original name in BTF (and in memory, fetchable through
> bpf_map__name()). So from the user's perspective that map is still
> named "blah-blah-something-long", even if the kernel records just a
> prefix of that.
>
> Basically, instead of forcing users to count the first 15 characters,
> warn, but do the right thing anyways?

Yes, you're right, i'll submit v3.

Rong Tao

>
>> Thank you!