[RFC] [PATCH bpf-next 0/1] Enhancement for bpf_do_path() helper

Background:
--------------------------------------------------------------------------------------------------
This is our scenario:
We restrict process to do certain operations in LSM hookpoint based on user specified security rules, 
and one dimension of the rule is the process’s executable full path, so we use bpf_d_path() helper to
get the path, and do rule matching in hash maps to make the allow or deny decision. However, the
returned buffer is not our expected, there is always noisy data at the tail of the buffer and makes
hash map lookup fails to work.
We have tried several ways to workaround this problem, such as using __builtin_memset() to clear
the returned buffer, but it does not work because __builtin_memset() cannot accept variable sized
buffer; and we also tried use a loop the iterate every byte of the buffer and clear the noisy data,
this works in simple bpf programs but makes a lot of trouble to us for complex bpf program cases,
e.g. reaching 1M instruction limitation or stack overflow.
Thus, we want to enhance the origin bpf_do_path() implementation, clearing the noisy tail buffer
helps a lot to us.

Appreciate for your suggestion.

Xufeng Zhang (1):
bpf: Clear the noisy tail buffer for bpf_d_path() helper
---
 kernel/trace/bpf_trace.c | 2 ++
 1 file changed, 2 insertions(+)

From: "Xufeng Zhang" <redacted>

The motivation behind this change is to use the returned full path
for lookup keys in BPF_MAP_TYPE_HASH map.
bpf_d_path() prepend the path string from the end of the input
buffer, and call memmove() to copy the full path from the tail
buffer to the head of buffer before return. So although the
returned buffer string is NULL terminated, there is still
noise data at the tail of buffer.
If using the returned full path buffer as the key of hash map,
the noise data is also calculated and makes map lookup failed.
To resolve this problem, we could memset the noisy tail buffer
before return.

Signed-off-by: Xufeng Zhang <redacted>
---
 kernel/trace/bpf_trace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 25ea521fb8f1..ec4a6823c024 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c

@@ -903,6 +903,8 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
 	} else {
 		len = buf + sz - p;
 		memmove(buf, p, len);
+		/* Clear the noisy tail buffer before return */
+		memset(buf + len, 0, sz - len);
 	}
 
 	return len;

-- 
2.20.1 (Apple Git-117)

Jiri and KP,

Any suggestion?


Thanks in advance!

Xufeng

在 2021/11/20 下午1:18, Xufeng Zhang 写道:

Hi,

On 11/24/2021 12:15 PM, xufeng zhang wrote:

Jiri and KP,

Any suggestion?


Thanks in advance!

Xufeng

在 2021/11/20 下午1:18, Xufeng Zhang 写道:

quoted

From: "Xufeng Zhang" <redacted>

The motivation behind this change is to use the returned full path
for lookup keys in BPF_MAP_TYPE_HASH map.
bpf_d_path() prepend the path string from the end of the input
buffer, and call memmove() to copy the full path from the tail
buffer to the head of buffer before return. So although the
returned buffer string is NULL terminated, there is still
noise data at the tail of buffer.
If using the returned full path buffer as the key of hash map,
the noise data is also calculated and makes map lookup failed.
To resolve this problem, we could memset the noisy tail buffer
before return.

Signed-off-by: Xufeng Zhang <redacted>
---
  kernel/trace/bpf_trace.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 25ea521fb8f1..ec4a6823c024 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c

@@ -903,6 +903,8 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf,

u32, sz)
      } else {
          len = buf + sz - p;
          memmove(buf, p, len);
+        /* Clear the noisy tail buffer before return */
+        memset(buf + len, 0, sz - len);

Is implementing bpf_memset() helper a better idea ? So those who need to
clear the buffer after the terminated null character can use the helper to
do that.

Regards,
Tao

quoted

      }
        return len;

.

Hi Tao,

在 2021/11/24 下午4:49, Hou Tao 写道:

Hi,

On 11/24/2021 12:15 PM, xufeng zhang wrote:

quoted

Jiri and KP,

Any suggestion?


Thanks in advance!

Xufeng

在 2021/11/20 下午1:18, Xufeng Zhang 写道:

quoted

From: "Xufeng Zhang" <redacted>

The motivation behind this change is to use the returned full path
for lookup keys in BPF_MAP_TYPE_HASH map.
bpf_d_path() prepend the path string from the end of the input
buffer, and call memmove() to copy the full path from the tail
buffer to the head of buffer before return. So although the
returned buffer string is NULL terminated, there is still
noise data at the tail of buffer.
If using the returned full path buffer as the key of hash map,
the noise data is also calculated and makes map lookup failed.
To resolve this problem, we could memset the noisy tail buffer
before return.

Signed-off-by: Xufeng Zhang <redacted>
---
   kernel/trace/bpf_trace.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 25ea521fb8f1..ec4a6823c024 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c

@@ -903,6 +903,8 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf,

u32, sz)
       } else {
           len = buf + sz - p;
           memmove(buf, p, len);
+        /* Clear the noisy tail buffer before return */
+        memset(buf + len, 0, sz - len);

Is implementing bpf_memset() helper a better idea ? So those who need to
clear the buffer after the terminated null character can use the helper to
do that.

This is a good point.

I think the reason why mainline has not such a helper yet is because a 
LLVM __builtin_memset() is

already available, but clearly this __builtin_memset() has too much 
limitation which can't meet all the needs,

there might be other concerns to implement such a memset helper which I 
don't know, but I think your suggestion

is a good idea.


Xufeng

Regards,
Tao

quoted

quoted

       }
         return len;

.