fix(drivers): add always a null terminator after args and envs
#1800
Conversation
Instruct `push__charbuf` to send a single `\0` when the unsafe pointer points to an empty string `""`. Doing it inside `push__charbuf` avoid extra ad-hoc code in various places Signed-off-by: Andrea Terzolo <[email protected]>
This commit handles `args` >= 4096. We always add a `\0` at the end of the args even in case of partial reads. Moreover this changes uniform the modern probe behavior with other drivers, before this commit we have a max len of 4096 for `exe` and other `4096` for args. After this change `exe + args` should be <= 4096 Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
|
Please double check driver/SCHEMA_VERSION file. See versioning. /hold |
poiana
added
size/XXL
do-not-merge/hold
kind/bug
| return 0; | ||
| } | ||
|
|
||
| /* Since `bpf_probe_read_user_str` return `0` in case of empty string we push a `\0` and we return 1. */ | ||
| if(written_bytes==0) |
There was a problem hiding this comment.
in case of an empty string, we send to userspace "" instead of an empty param with len 0
| @@ -214,7 +214,8 @@ static __always_inline void push__previous_character(uint8_t *data, uint64_t *pa | |||
| * @param charbuf_pointer pointer to the charbuf. | |||
| * @param limit maximum number of bytes that we read in case we don't find a `\0` | |||
| * @param mem tell where it must read: user-space or kernel-space. | |||
| * @return (uint16_t) the number of bytes written in the buffer. Could be '0' if the passed pointer is not valid. | |||
| * @return (uint16_t) the number of bytes written in the buffer. Returns '0' if the passed pointer is not valid. | |||
| * Returns `1` if the provided pointer points to an empty string "". | |||
There was a problem hiding this comment.
before this patch we returned 0 and so we sent a param with len 0 to userspace
| */ | ||
| if(auxmap__store_bytebuf_param(auxmap, start_pointer, len_to_read, USER) > 0) | ||
| { | ||
| // maybe we read only part of the last argument so we need to put a `\0` at the end. |
There was a problem hiding this comment.
if auxmap__store_bytebuf_param returned 0 we don't need to push a null terminator
| * @param auxmap pointer to the auxmap in which we are storing the param. | ||
| * @param charbuf pointer array, obtained directly from the syscall (`argv`). | ||
| */ | ||
| static __always_inline void auxmap__store_exe_args_failure(struct auxiliary_map *auxmap, char **array) |
There was a problem hiding this comment.
this is a pretty ugly helper that pushes together 2 params, the exe and the args. This is done to be compliant with other drivers (see tests). all the other drivers share a unique limit of 4096 bytes for exe and args, while in the modern, before this patch we could have an exe of 4096 + an args of 4096
|
|
||
| args_len += push__charbuf(auxmap->data, &auxmap->payload_pos, charbuf_pointer, MAX_PROC_ARG_ENV, USER); | ||
|
|
||
| /* the sum of `exe` + `args` should be `<= MAX_PROC_ARG_ENV` */ |
There was a problem hiding this comment.
like in the other drivers
| * @param auxmap pointer to the auxmap in which we are storing the param. | ||
| * @param charbuf pointer array, obtained directly from the syscall (`argv`). | ||
| */ | ||
| static __always_inline void auxmap__store_exe_args_failure(struct auxiliary_map *auxmap, char **array) |
There was a problem hiding this comment.
this is called failure becuase it is used to collect exe and args only in case of failures
| if(args_len > 0) | ||
| { | ||
| auxmap->payload_pos = initial_payload_pos + args_len; | ||
| push__previous_character(auxmap->data, &auxmap->payload_pos, '\0'); |
There was a problem hiding this comment.
always null terminated
| @@ -396,25 +465,18 @@ static __always_inline void auxmap__store_execve_args(struct auxiliary_map *auxm | |||
| } | |||
|
|
|||
| arg_len = push__charbuf(auxmap->data, &auxmap->payload_pos, charbuf_pointer, MAX_PROC_ARG_ENV, USER); | |||
There was a problem hiding this comment.
now that push__charbuf handles empty strings we can simplify the flow
|
|
||
| if (unlikely(ppm_get_user(p, argv))) | ||
| return PPM_FAILURE_INVALID_USER_MEMORY; |
There was a problem hiding this comment.
this caused an event drop in case of failure
| @@ -991,8 +987,8 @@ int f_proc_startupdate(struct event_filler_arguments *args) | |||
| args_len = mm->arg_end - mm->arg_start; | |||
|
|
|||
| if (args_len) { | |||
| if (args_len > PAGE_SIZE) | |||
| args_len = PAGE_SIZE; | |||
| if (args_len > STR_STORAGE_SIZE) | |||
There was a problem hiding this comment.
STR_STORAGE_SIZE is PAGE_SIZE but if one day they should diverge is better to use STR_STORAGE_SIZE everywhere
| args_len = accumulate_argv_or_env((const char __user * __user *)val, | ||
| args->str_storage, available); | ||
|
|
||
| if (unlikely(args_len < 0)) |
There was a problem hiding this comment.
this is no more possible, we cannot return an args_len < 0
Andreagit97
changed the title
args and envsargs and envs
|
there is still an issue with the legacy ebpf probe, I'm trying to understand what is going on... |
Signed-off-by: Andrea Terzolo <[email protected]>
Andreagit97
force-pushed
the
fix_modern_bpf_probe
branch
from
04590f1 to
bfca262
Compare
| if(argv == NULL) | ||
| { | ||
| // we need to put a `\0` otherwise we could read junk data | ||
| data->buf[off & SCRATCH_SIZE_HALF] = '\0'; |
There was a problem hiding this comment.
this is why the CI was failing, if we called execve with argv or 'env NULLwe ended up reading random memory indata->buf` written by the previous process on this CPU.
|
It seems there are no regressions in the kernel testing: x86
arm64
|
|
LGTM label has been added. Git tree hash: 2c78ee4f425665ed0aa2c7a546859523b7851f2c
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, LucaGuerra The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area driver-kmod
/area driver-modern-bpf
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
This PR contains multiple fixes:
"") are not collected in the same way between our drivers #866. Sincebpf_probe_read_user_strreturns0in case of an empty string we now add manually a null terminator directly inpush__charbuf, in this case we can handle empty strings in just one place. Partially reverting this fix(driver/bpf): not sending all arguments on execve fail #1760.argsandenvsgreater than the limit (4096). Before this fix, in the case ofargs>= 4096, they were truncated without adding a\0at the end. This causes issues when handling these params in userspace. Now even in case of partial reads we always terminate the args with a final\0.accumulate_argv_or_env. Before this fix, in case of page faults or other errors, the entire event was dropped, now in case of errors we send partial args null-terminated.Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: