fix(driver/bpf): fixed a couple of verifier issues. #1896
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -571,236 +571,158 @@ FILLER(sys_poll_x, true) | |
| return bpf_poll_parse_fds(data, false); | ||
| } | ||
|
|
||
| #ifdef CONFIG_COMPAT | ||
| #define MAX_IOVCNT 8 | ||
| #else | ||
| #define MAX_IOVCNT 32 | ||
| #endif | ||
|
|
||
| static __always_inline int bpf_parse_readv_writev_bufs(struct filler_data *data, | ||
|
There was a problem hiding this comment. to be honest I would prefer to revert this code to what we had before this PR https://github.com/falcosecurity/libs/pull/1814/files#diff-a8b6fc07077f8fcef2f22e51392da619a5e68b7c603c62b3ab93ce5b6be22d6b. The new implementation seems pretty easy to break and the only advantage is that we support Btw if you feel confident we can also merge it and maybe revert it if we see it doesn't work well... There was a problem hiding this comment. Yep, i would love to be able to just use |
||
| const void __user *iovsrc, | ||
| unsigned long iovcnt, | ||
| long retval, | ||
| int flags) | ||
| { | ||
| int res = PPM_SUCCESS; | ||
| unsigned long copylen; | ||
| long size = 0; | ||
| int j; | ||
| unsigned long iov_size = sizeof(struct iovec); | ||
| unsigned long len_off = offsetof(struct iovec, iov_len); | ||
| unsigned long base_off = offsetof(struct iovec, iov_base); | ||
| unsigned long ptr_size = sizeof(void *); | ||
|
|
||
| #ifdef CONFIG_COMPAT | ||
| if (bpf_in_ia32_syscall()) | ||
| { | ||
| iov_size = sizeof(struct compat_iovec); | ||
| len_off = offsetof(struct compat_iovec, iov_len); | ||
| base_off = offsetof(struct compat_iovec, iov_base); | ||
| ptr_size = 4; | ||
| } | ||
| #endif | ||
|
|
||
| copylen = iovcnt * iov_size; | ||
| if (copylen > SCRATCH_SIZE_MAX) | ||
| { | ||
| return PPM_FAILURE_FRAME_SCRATCH_MAP_FULL; | ||
| } | ||
|
|
||
| #ifdef BPF_FORBIDS_ZERO_ACCESS | ||
| if (copylen) | ||
| if (bpf_probe_read_user(data->tmp_scratch, | ||
| ((copylen - 1) & SCRATCH_SIZE_MAX) + 1, | ||
| (void *)iovsrc)) | ||
| #else | ||
| if (bpf_probe_read_user(data->tmp_scratch, | ||
| copylen & SCRATCH_SIZE_MAX, | ||
| (void *)iovsrc)) | ||
| #endif | ||
| return PPM_FAILURE_INVALID_USER_MEMORY; | ||
|
|
||
|
|
||
| #pragma unroll | ||
| for (j = 0; j < MAX_IOVCNT; ++j) { | ||
| if (j == iovcnt) | ||
| break; | ||
| // BPF seems to require a hard limit to avoid overflows | ||
| if (size == LONG_MAX) | ||
| break; | ||
|
|
||
| volatile unsigned curr_shift = j * iov_size + len_off; | ||
| unsigned long shift_bounded = curr_shift & SCRATCH_SIZE_HALF; | ||
| if (curr_shift > SCRATCH_SIZE_HALF) | ||
| break; | ||
|
|
||
| long curr_len; | ||
|
There was a problem hiding this comment. Don't know why, it won't make me use There was a problem hiding this comment. If i could use There was a problem hiding this comment. Oh we can't because for |
||
| if (ptr_size == 4) | ||
| { | ||
| curr_len = *((int *)(data->tmp_scratch + shift_bounded)); | ||
| } | ||
| else | ||
| { | ||
| curr_len = *((long *)(data->tmp_scratch + shift_bounded)); | ||
| } | ||
| size += curr_len; | ||
| } | ||
|
|
||
| if ((flags & PRB_FLAG_IS_WRITE) == 0) | ||
| if (size > retval) | ||
| size = retval; | ||
|
|
||
| if (flags & PRB_FLAG_PUSH_SIZE) { | ||
| res = bpf_push_u32_to_ring(data, (uint32_t)size); | ||
| CHECK_RES(res); | ||
| } | ||
|
|
||
| if (flags & PRB_FLAG_PUSH_DATA) { | ||
| if (size > 0) { | ||
| unsigned long off = _READ(data->state->tail_ctx.curoff); | ||
| unsigned long remaining = size; | ||
|
|
||
| #pragma unroll | ||
| for (j = 0; j < MAX_IOVCNT; ++j) { | ||
| volatile unsigned int to_read; | ||
| if (j == iovcnt) | ||
| break; | ||
| unsigned long off_bounded = off & SCRATCH_SIZE_HALF; | ||
| if (off > SCRATCH_SIZE_HALF) | ||
| break; | ||
|
|
||
| volatile unsigned len_curr_shift = j * iov_size + len_off; | ||
| unsigned long len_shift_bounded = len_curr_shift & SCRATCH_SIZE_HALF; | ||
| if (len_curr_shift > SCRATCH_SIZE_HALF) | ||
| break; | ||
|
|
||
| long curr_len; | ||
| if (ptr_size == 4) | ||
| { | ||
| curr_len = *((int *)(data->tmp_scratch + len_shift_bounded)); | ||
| } | ||
| else | ||
| { | ||
| curr_len = *((long *)(data->tmp_scratch + len_shift_bounded)); | ||
| } | ||
| if (curr_len <= remaining) | ||
| to_read = curr_len; | ||
| else | ||
| to_read = remaining; | ||
| if (to_read > SCRATCH_SIZE_HALF) | ||
| to_read = SCRATCH_SIZE_HALF; | ||
|
|
||
| volatile unsigned base_curr_shift = j * iov_size + base_off; | ||
| unsigned long base_shift_bounded = base_curr_shift & SCRATCH_SIZE_HALF; | ||
| if (base_curr_shift > SCRATCH_SIZE_HALF) | ||
| break; | ||
|
|
||
| unsigned long curr_base; | ||
| if (ptr_size == 4) | ||
| { | ||
| curr_base = *((unsigned int *)(data->tmp_scratch + base_shift_bounded)); | ||
| } | ||
| else | ||
| { | ||
| curr_base = *((unsigned long *)(data->tmp_scratch + base_shift_bounded)); | ||
| } | ||
| #ifdef BPF_FORBIDS_ZERO_ACCESS | ||
| if (to_read) | ||
| if (bpf_probe_read_user(&data->buf[off_bounded], | ||
| ((to_read - 1) & SCRATCH_SIZE_HALF) + 1, | ||
| (void *)curr_base)) | ||
| #else | ||
| if (bpf_probe_read_user(&data->buf[off_bounded], | ||
| to_read & SCRATCH_SIZE_HALF, | ||
| (void *)curr_base)) | ||
| #endif | ||
| return PPM_FAILURE_INVALID_USER_MEMORY; | ||
|
|
||
| remaining -= to_read; | ||
| off += to_read; | ||
| } | ||
| } else { | ||
| size = 0; | ||
| } | ||
|
|
||
| data->fd = bpf_syscall_get_argument(data, 0); | ||
| data->curarg_already_on_frame = true; | ||
| return __bpf_val_to_ring(data, 0, size, PT_BYTEBUF, -1, true, KERNEL); | ||
|
|
@@ -4312,10 +4234,12 @@ FILLER(sys_recvfrom_x, true) | |
| unsigned long val; | ||
| uint16_t size = 0; | ||
| long retval; | ||
| int addrlen = 0; | ||
|
There was a problem hiding this comment.
|
||
| int err = 0; | ||
| int res; | ||
| int fd; | ||
| bool push = true; | ||
| bool from_usr = false; | ||
|
|
||
| /* | ||
| * Push the common params to the ring | ||
|
|
@@ -4324,6 +4248,7 @@ FILLER(sys_recvfrom_x, true) | |
| res = f_sys_recv_x_common(data, retval); | ||
| CHECK_RES(res); | ||
|
|
||
|
|
||
| if (retval >= 0) { | ||
| /* | ||
| * Get the fd | ||
|
|
@@ -4350,29 +4275,26 @@ FILLER(sys_recvfrom_x, true) | |
| */ | ||
| err = bpf_addr_to_kernel(usrsockaddr, addrlen, | ||
| (struct sockaddr *)data->tmp_scratch); | ||
| if (err >= 0) | ||
| { | ||
| /* | ||
| * Convert the fd into socket endpoint information | ||
| */ | ||
| from_usr = true; | ||
| } | ||
| else | ||
| { | ||
| // Do not send any socket endpoint info. | ||
| push = false; | ||
| } | ||
| } | ||
| if (push) | ||
| { | ||
| /* | ||
| * Get socket endpoint information from fd if the user-provided *sockaddr is NULL | ||
| */ | ||
| size = bpf_fd_to_socktuple(data, fd, (struct sockaddr *)data->tmp_scratch, addrlen, from_usr, | ||
| true, data->tmp_scratch + sizeof(struct sockaddr_storage)); | ||
| } | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only real downside of the patch (except for its ugliness probably :D ) is that we will have max iovec count of 8 when config compat is enabled; this is needed because we must have a constant to be able to unroll the loops below. Not a big deal; this is only on the old ebpf probe.