Fix daemon SEGV when invalid chain type is passed

[pzmarzly]

I was playing with bpfilter, and typo-ed a bfcli command. To my surprise, it caused a SIGSEGV crash in daemon process. That's because bfcli parser returned -22 (EINVAL), which was passed as-is to the daemon, which used it to index into an array, leading to out-of-bounds array access.

I'm introducing 3 changes here:

• libbpfilter: Add some out-of-bounds checks - adding some bf_assert checks in helper functions that read from arrays, so that in debug builds we get a cleaner signal
• bfcli: Fix parsing unknown hook types - instead of passing EINVAL along, print an error to the user and abort
• daemon: Reject invalid hook types - some hardening on server side, to turn this crash into an error with easy-to-understand message

[qdeslandes]

About the commits titles:

• libbpfilter: Add some out-of-bounds checks -> lib: add some out-of-bounds checks
• bfcli: Fix parsing unknown hook types -> cli: fix parsing unknown hook types
• daemon: Reject invalid hook types -> daemon: reject invalid hook types

Most of those comments are due to stuff that is not properly formalized and described in the doc, mostly because is evolved over time (hence, not applied to the whole codebase).

[qdeslandes]

Not necessary. bf_hook_from_str() returns a enum bf_hook type, we should store it as such. return -EINVAL is a special error case which can be dealt with in using (hook < 0) even if hook is enum bf_hook type.

[pzmarzly]

Sadly, seems necessary. Storing negative values in enum is UB, and GCC optimizes that if away, even in -O0.

Godbolt: https://godbolt.org/z/MvWzob36e
Meta internal explanation: M8N33B

[qdeslandes]

Oh no :(

Alright, let's find a different approach then. Another approach could be:
int bf_hook_from_str(const char *str, enum bf_hook *ret). This way we have a return value to check, and we can use ret on success. It's a bit more verbose, but at least our back's covered.

The other option would be to use int bf_hook_from_str(const char *str) and cast the return to enum bf_hook after it's checked. I find the suggestion above better as there's clear distinction between the return value and the parsed value. What do you think?

Oooooor, we can return the sentinel value on error: _BF_HOOK_MAX if the string can't be parsed.

[pzmarzly]

int bf_hook_from_str(const char *str, enum bf_hook *ret)

I like this approach the most. Now I'm thinking about redoing these commits from scratch, and adding validation only to the two points where we accept user input:

• for CLI, change bf_hook_from_str like you described
• for daemon, change bf_rpack_kv_enum(node, key, value) to bf_rpack_kv_enum(node, key, value, min, max) and validate the input value when parsing the request instead of later

Seems other parsing, for example bf_matcher_type_from_str, already uses that approach.

[qdeslandes]

Oh no :(

Alright, let's find a different approach then. Another approach could be:
int bf_hook_from_str(const char *str, enum bf_hook *ret). This way we have a return value to check, and we can use ret on success. It's a bit more verbose, but at least our back's covered.

The other option would be to use int bf_hook_from_str(const char *str) and cast the return to enum bf_hook after it's checked. I find the suggestion above better as there's clear distinction between the return value and the parsed value. What do you think?

Oooooor, we can return the sentinel value on error: _BF_HOOK_MAX if the string can't be parsed.

[pzmarzly]

Closing in favor of #366 stack