Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(driver/bpf): bpf_addr_to_kernel takes an unsigned length (less than BPF_MAX_VAR_SIZE) #1730

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

fix(driver/bpf): bpf_addr_to_kernel takes an unsigned length (less than BPF_MAX_VAR_SIZE) #1730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9caa409
fix(driver/bpf): bpf_addr_to_kernel takes an unsigned length (less than
leodido Jan 14, 2021
4c2353a
docs(driver/bpf): document the source of BPF_MAX_VAR_SIZE
leodido Jan 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions driver/bpf/filler_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ or GPL2.txt for full copies of the license.

#include "../ppm_flag_helpers.h"

// Maximum var size allowed by the verifier
leodido marked this conversation as resolved.
Show resolved Hide resolved
// From BPF_MAX_VAR_SIZ in linux/bpf_verifier.h
#define BPF_MAX_VAR_SIZE 1ULL << 29

static __always_inline bool in_port_range(uint16_t port, uint16_t min, uint16_t max)
{
return port >= min && port <= max;
Expand Down Expand Up @@ -212,10 +216,10 @@ static __always_inline bool bpf_getsockname(struct socket *sock,
return true;
}

static __always_inline int bpf_addr_to_kernel(void *uaddr, int ulen,
static __always_inline int bpf_addr_to_kernel(void *uaddr, unsigned long ulen,
struct sockaddr *kaddr)
{
if (ulen < 0 || ulen > sizeof(struct sockaddr_storage))
if (ulen > BPF_MAX_VAR_SIZE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to increase the valid range of ulen. Previously the function would return -EINVAL if ulen > sizeof(struct sockaddr_storage), but on my machine that sizeof operation resolves to 128, which is far, far less than BPF_MAX_VAR_SIZE. Is this intended?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're doing ulen & 0xff (more or less) below. I feel that we should return an error whenever ulen > 0xff (otherwise the caller can request e.g. 0x1ff bytes--the value comes straight from userspace--while we copy 0xff and return success).

The original check against sizeof(struct sockaddr_storage) feels more correct.

Also, not sure what exactly BPF_FORBIDS_ZERO_ACCESS implies but if my guess (length must be > 0) is correct, both the #ifdef and the & 0xff (if we revert the size check) is unnecessary (ulen will always be > 0 after line 224 and < 0xff after original line 218).

Though I expect both are there to keep the verifier happy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That check seems like a leftover from move_addr_to_kernel kernel function.

Anyways, we read at maximum ulen & 0xff every time (see bpf_probe_read below).

After some intensive and day-long testing, we (@fntlnz and I) have not been able to break it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnosek The verifier rejects a check against sizeof(struct sockaddr_storage).

Copy link
Contributor Author

@leodido leodido Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway the bug here was about the int (now unsigned long) parameter and the missing boundary check.

Both were required by the verifier (giving two different errors on each of them). Thus, this is the scope of this PR.

In case we'd like to optimize the behaviour of the mask and the presence (or not) of ifdefs, we should approach those in another PR, imho.

As per our testing, they do not interfere with the correct functioning of the probe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you replace BPF_MAX_VAR_SIZE with sizeof(struct sockaddr_storage), the verifier fails?

That is bizarre. That seems like a bug in the verifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we tried in many many ways to retain that check too, but the verifier rejects it.

return -EINVAL;

if (ulen == 0)
Expand Down Expand Up @@ -289,7 +293,7 @@ static __always_inline u32 bpf_compute_snaplen(struct filler_data *data,
if (!bpf_getsockname(sock, peer_address, 1))
return res;
} else {
int addrlen = bpf_syscall_get_argument(data, 5);
unsigned long addrlen = bpf_syscall_get_argument(data, 5);

if (addrlen != 0) {
if (bpf_addr_to_kernel(usrsockaddr, addrlen, (struct sockaddr *)peer_address))
Expand All @@ -302,7 +306,7 @@ static __always_inline u32 bpf_compute_snaplen(struct filler_data *data,
struct sockaddr *usrsockaddr;
struct user_msghdr mh;
unsigned long val;
int addrlen;
unsigned long addrlen;

val = bpf_syscall_get_argument(data, 1);
if (bpf_probe_read(&mh, sizeof(mh), (void *)val)) {
Expand Down
6 changes: 3 additions & 3 deletions driver/bpf/fillers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2640,7 +2640,7 @@ FILLER(sys_recvfrom_x, true)
unsigned long val;
u16 size = 0;
long retval;
int addrlen;
unsigned long addrlen;
int err = 0;
int res;
int fd;
Expand Down Expand Up @@ -2770,7 +2770,7 @@ FILLER(sys_recvmsg_x_2, true)
unsigned long val;
u16 size = 0;
long retval;
int addrlen;
unsigned long addrlen;
int res;
int fd;

Expand Down Expand Up @@ -2832,7 +2832,7 @@ FILLER(sys_sendmsg_e, true)
unsigned long iovcnt;
unsigned long val;
u16 size = 0;
int addrlen;
unsigned long addrlen;
int err = 0;
int res;
int fd;
Expand Down