Details

Reviewers

jamie
glebius
melifaro
kp

Commits

rG500f82d6c32e: jail: Use flexible array member within struct prison_ip

Summary

Current implementation utilize off-by-one struct prison_ip to access the IPv[46] addresses. It is error prone and hence comes the regression fix D37732, D37871 and D37872 . Use flexible array member so that compiler will catch the errors and it is also easier to review.

No functional change intended.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

zlei created this revision.Dec 25 2022, 6:59 PM

Herald added a subscriber: imp. · View Herald TranscriptDec 25 2022, 6:59 PM

zlei requested review of this revision.Dec 25 2022, 6:59 PM

zlei added parent revisions: D37872: jail: Properly restrict the child jail's IPv[46] addresses, D37871: jail: Fix wrong IPv[46] addresses inherited from parent jail.

This is a request by @melifaro in D37732 .

In D37732#858183, @melifaro wrote:

LGTM!
Do you think it would be possible to convert PR_IP to be static inline function, so that kind of erros are spotted by the compiler?

I'll take a look at that. Smart compilers help a lot ;)

zlei mentioned this in D37732: jail: Fix output of IPv[46] addresses of DDB `show prison`.Dec 25 2022, 7:04 PM

In D37874#860475, @zlei wrote:

This is a request by @melifaro in D37732 .

In D37732#858183, @melifaro wrote:

LGTM!
Do you think it would be possible to convert PR_IP to be static inline function, so that kind of erros are spotted by the compiler?

I'll take a look at that. Smart compilers help a lot ;)

Thank you!

sys/kern/kern_jail.c
578–582	Is this still needed?
629	‘pr_ip’ is an array. IIRC the typical pattern is either ‘arr’ or ‘&arr[0]’

zlei added inline comments.Dec 27 2022, 1:34 AM

sys/kern/kern_jail.c
578–582	Is this still needed? Variable length arrays are allowed in ISO C99. They are standard and more elegant. Zero-length arrays in struct / union is not allowed in ISO C but extensions of GCC (prior ISO C) and Clang. I'd prefer ISO ones as maybe the extension (of zero-length arrays) will be disabled and ISO ones is enforced in future. For now I'll remove it and leave a comment instead.
629	Good catch! This is a middle night work and `&arr` is indeed wrong. Well `&arr` point to the same address as `arr` or `&arr[0]`, but it has different type. `&arr + 1 != arr + 1` .

Address @melifaro 's comment.

zlei marked 2 inline comments as done.Dec 27 2022, 3:42 AM

Reduce usage of parentheses .

melifaro accepted this revision.Dec 27 2022, 8:40 PM

melifaro added inline comments.

sys/kern/kern_jail.c
639	Nit: maybe that's more readable?

This revision is now accepted and ready to land.Dec 27 2022, 8:40 PM

zlei added inline comments.Dec 28 2022, 1:32 AM

sys/kern/kern_jail.c
639	Yes I think so.
832	`&ppip->pr_ip` -> `ppip->pr_ip`

zlei updated this revision to Diff 114589.Dec 28 2022, 2:57 AM

This revision now requires review to proceed.Dec 28 2022, 2:57 AM

zlei added a child revision: D37890: jail: Improve readability.Dec 28 2022, 3:54 AM

zlei marked an inline comment as done.Dec 28 2022, 4:00 AM

zlei added inline comments.

sys/kern/kern_jail.c
639	Nit: maybe that's more readable? Create separate differential D37890 so it will be easy to review.

zlei mentioned this in D37871: jail: Fix wrong IPv[46] addresses inherited from parent jail.Dec 28 2022, 5:15 AM

melifaro added inline comments.Dec 28 2022, 5:49 PM

sys/kern/kern_jail.c
2378	If `pr->pr_addrs[PR_INET]` is NULL (as the line below suggest) won't it crash? Same for IPv6 case

melifaro added inline comments.Dec 28 2022, 6:07 PM

sys/kern/kern_jail.c
592	Maybe it's worth converting the macro to the function, so we get type checking & argument asserts for free?

zlei marked an inline comment as done.Dec 29 2022, 3:43 PM

zlei added inline comments.

sys/kern/kern_jail.c
2378	No, it won't crash. Checked the implementation of `vfs_setopt_part(struct vfsoptlist opts, const char name, void *value, int len)`, it internally calls `bcopy(value, opt->value, len)`. As per BCOPY(3), The bcopy() function copies len bytes from string src to string dst. The two strings may overlap. If len is zero, no bytes are copied. If `pr->pr_addrs[PR_INET(6)]` is null then `len` will be `zero`, and `bcopy()` will behave as a nop.

zlei marked an inline comment as done.Dec 29 2022, 4:11 PM

zlei added inline comments.

sys/kern/kern_jail.c
592	That make sense.

melifaro added inline comments.Dec 29 2022, 4:58 PM

sys/kern/kern_jail.c
2378	Mm. I meant that in order to call `vfs_setopt_part()`, the code has to resolve its arguments first. It is possible for `pr->pr_addrs[PR_INET]` to be `NULL`. My question was what happens with the pr->pr_addrs[PR_INET]->pr_ip4 argument

zlei added inline comments.Dec 29 2022, 5:07 PM

sys/kern/kern_jail.c

592

Maybe it's worth converting the macro to the function, so we get type checking & argument asserts for free?

Compiler complains (char *) to (const char*) converting. So I have to define two functions.

static const char *
pr_ip_get(const struct prison_ip *pip, const pr_family_t af, int idx)
{
	MPASS(pip);
	MPASS(af < PR_FAMILY_MAX);
	MPASS(idx >= 0);

	return (pip->pr_ip + pr_families[af].size * idx);
}

static char *
pr_ip_get_d(struct prison_ip *pip, const pr_family_t af, int idx)
{
	MPASS(pip);
	MPASS(af < PR_FAMILY_MAX);
	MPASS(idx >= 0);

	return (pip->pr_ip + pr_families[af].size * idx);
}

#define PR_IP(pip, i)	(pr_ip_get((pip), af, (i)))
#define PR_IPD(pip, i)	(pr_ip_get_d((pip), af, (i)))

That seems a bit of overkill .
@melifaro What do you think ?

melifaro added inline comments.Dec 29 2022, 5:22 PM

sys/kern/kern_jail.c
592	Well, I'd still prefer to have them. In the prod version they'll effectively be the same one-liner, but we'll get a lot of validity checks. If we had this before, the issue spotted in D37872 ( case 0: bcopy(PR_IP(pr, i), PR_IPD(new, ips), size); ) would be caught by the compiler.

zlei marked an inline comment as done.Dec 29 2022, 5:51 PM

zlei added inline comments.

sys/kern/kern_jail.c
2378	Ooh, my fault. For short no side effect. Although `pr->pr_addrs[PR_INET]` might be `NULL` but we are not dereferencing `NULL` pointer. It looks tricky but `pr_ip4` is an (zero sized) array. `pr->pr_addrs[PR_INET]->pr_ip4` is equivalent to `&pr->pr_addrs[PR_INET]->pr_ip4[0]` and `(struct in_addr )((char )(pr->pr_addrs[PR_INET]) + (sizeof(struct prison_ip) - sizeof(pr_ip[0]))` . That is the same with `pr->pr_addrs[PR_INET] + 1` . I'll demonstrate that online https://godbolt.org/z/f4hq63rdj

melifaro added inline comments.Dec 29 2022, 5:55 PM

sys/kern/kern_jail.c
2378	Yep. My bad. For some reason, the idea that `pr_ip4` has a separate pointer was stuck in my head. Thank you, no problem here. Thank you for the patience :-)

Define macros as functions

zlei marked 3 inline comments as done.Dec 29 2022, 5:59 PM

melifaro accepted this revision.Dec 29 2022, 6:00 PM

This revision is now accepted and ready to land.Dec 29 2022, 6:00 PM

This should rebase onto D37906. I will commit and do that after D37906 is landed.

There are reasons why I avoid using the GCC extension (although supported by Clang) and used standard C. The reasons are spread around the Internet, but the best sum up of the reasoning can be found in the Linux kernel guidelines :)

https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

So the GNU worlds itself gets rid of this GCC-ism. See Linux commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=5224f79096170bf7b92cc8fe42a12f44b91e5f62

Is it possible to fix the bugs without [0]?

This revision now requires changes to proceed.Dec 30 2022, 5:02 AM

In D37874#861264, @glebius wrote:

There are reasons why I avoid using the GCC extension (although supported by Clang) and used standard C. The reasons are spread around the Internet, but the best sum up of the reasoning can be found in the Linux kernel guidelines :)

https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

So the GNU worlds itself gets rid of this GCC-ism. See Linux commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=5224f79096170bf7b92cc8fe42a12f44b91e5f62

IIRC Linus Torvalds decided to move the Linux kernel from C89 to modern C11 standard. I'm not familiar with Linux kernel code so I
can not tell which commit enforced that.

This should happen after the enforcement.

Is it possible to fix the bugs without [0]?

Indeed they're fixed without [0]. See D37871 and D37872.

I'd prefer the ISO standards but FreeBSD is not ready for it.

PS: Any plans FreeBSD move to C99 or C11 ?

In D37874#861283, @zlei wrote:

In D37874#861264, @glebius wrote:

There are reasons why I avoid using the GCC extension (although supported by Clang) and used standard C. The reasons are spread around the Internet, but the best sum up of the reasoning can be found in the Linux kernel guidelines :)

https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

So the GNU worlds itself gets rid of this GCC-ism. See Linux commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=5224f79096170bf7b92cc8fe42a12f44b91e5f62

IIRC Linus Torvalds decided to move the Linux kernel from C89 to modern C11 standard. I'm not familiar with Linux kernel code so I
can not tell which commit enforced that.

This should happen after the enforcement.

Is it possible to fix the bugs without [0]?

Indeed they're fixed without [0]. See D37871 and D37872.

I'd prefer the ISO standards but FreeBSD is not ready for it.

PS: Any plans FreeBSD move to C99 or C11 ?

Ooh sorry for that. FreeBSD kernel source is build with C99.

Back to the problem,

Is it possible to fix the bugs without [0]?

Compiler complains if the members in union are flexible array,

/usr/src/sys/kern/kern_jail.c:580:8: error: flexible array member 'pr_ip' in a union is not allowed
                char pr_ip[];
                     ^
/usr/src/sys/kern/kern_jail.c:582:18: error: flexible array member 'pr_ip4' in a union is not allowed
                struct in_addr pr_ip4[];
                               ^
/usr/src/sys/kern/kern_jail.c:585:19: error: flexible array member 'pr_ip6' in a union is not allowed
                struct in6_addr pr_ip6[];

@glebius What about this one ?

struct prison_ip {
        struct epoch_context ctx;
        uint32_t        ips;
        union {
#ifdef INET
                struct in_addr pr_ip4[0];
#endif
#ifdef INET6
                struct in6_addr pr_ip6[0];
#endif
        };
        char pr_ip[];
};

That prevents problems of zero-length arrays and benefits variable-length arrays (flexible array) .

I think that zero-length arrays won't ever be admitted to the standard. However, I can imagine that a union of flexible ones (the code commented out) would be. That's why I don't want to add new code that heavily relies on zero-length arrays and rather prefer the ugly macros.

Address @glebius 's concern. Use flexible arrays.

Yes, this is better than what I did.

I think we need either name the function PR_IP() and skip macro, or save the name of the function as is, but convert usage to function name instead of macro name.

For the constness, I guess we can just drop the const version and have single non-const function. It was useful to catch errors at early stage of development of the code. Maybe not so useful today? What do you think?

Don't forget to change the title of the changeset when you commit.

This revision is now accepted and ready to land.Jan 10 2023, 7:45 PM

Address @glebius 's feedback.

Remove macro PR_IPD
And drop some const qualifiers

This revision now requires review to proceed.Feb 21 2023, 2:42 PM

Address @glebius 's comment, use function instead of macro PR_IP

zlei retitled this revision from jail: Use union of zero length arrays in struct prison_ip to jail: Use flexible array member within struct prison_ip.Feb 21 2023, 3:26 PM

zlei edited the summary of this revision. (Show Details)

This revision was not accepted when it landed; it landed in state Needs Review.Feb 28 2023, 10:21 AM

Closed by commit rG500f82d6c32e: jail: Use flexible array member within struct prison_ip (authored by zlei). · Explain Why

This revision was automatically updated to reflect the committed changes.

zlei added a commit: rG500f82d6c32e: jail: Use flexible array member within struct prison_ip.

jail: Use flexible array member within struct prison_ip
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 117687

sys/kern/kern_jail.c

jail: Use flexible array member within struct prison_ipClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 117687

sys/kern/kern_jail.c

jail: Use flexible array member within struct prison_ip
ClosedPublic
Actions

Revision Contents
Changeset List