On Mon, 2020-02-24 at 13:47 +0100, Ondrej Mosnacek wrote:
> First, the setting of SCTP_EVENTS socket option in sctp_server.c is
> completely wrong -- it assumes little-endian byte order and uses a
> plain
> int instead of the dedicated sctp_event_subscribe struct.
>
> Second, the usage in sctp_peeloff_server.c is correct, but it may
> lead
> to errors when the SCTP header definitions are newer than what the
> kernel supports. In such case the size of struct sctp_event_subscribe
> may be higher than the kernel's version and the setsockopt(2) may
> fail
> with -EINVAL due to the 'optlen > sizeof(struct
> sctp_event_subscribe)'
> check in net/sctp/socket.c:sctp_setsockopt_events().
>
> To fix this, introduce a common function that does what the
> sctp_peeloff_server's set_subscr_events() did, but also truncates the
> optlen to only those fields that we use.
Thanks for fixing these problems. How did the problems come to light,
just via the header file changes or some tests failing on a certain
kernel ?
>
> Fixes: c38b57ffdac4 ("selinux-testsuite: Add SCTP test support")
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> tests/sctp/sctp_common.c | 20 ++++++++++++++++++++
> tests/sctp/sctp_common.h | 1 +
> tests/sctp/sctp_peeloff_server.c | 28 ++++++++--------------------
> tests/sctp/sctp_server.c | 2 +-
> 4 files changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/tests/sctp/sctp_common.c b/tests/sctp/sctp_common.c
> index 100ab22..089af2a 100644
> --- a/tests/sctp/sctp_common.c
> +++ b/tests/sctp/sctp_common.c
> @@ -1,5 +1,8 @@
> #include "sctp_common.h"
>
> +#define member_size(type, member) sizeof(((type *)0)->member)
> +#define sizeof_up_to(type, member) (offsetof(type, member) +
> member_size(type, member))
> +
> void print_context(int fd, char *text)
> {
> char *context;
> @@ -99,3 +102,20 @@ void print_ip_option(int fd, bool ipv4, char
> *text)
> printf("%s No IP Options set\n", text);
> }
> }
> +
> +int set_subscr_events(int fd, int data_io, int association)
> +{
> + struct sctp_event_subscribe subscr_events;
> +
> + memset(&subscr_events, 0, sizeof(subscr_events));
> + subscr_events.sctp_data_io_event = data_io;
> + subscr_events.sctp_association_event = association;
> +
> + /*
> + * Truncate optlen to just the fields we touch to avoid errors
> when
> + * the uapi headers are newer than the running kernel.
> + */
> + return setsockopt(fd, IPPROTO_SCTP, SCTP_EVENTS,
> &subscr_events,
> + sizeof_up_to(struct sctp_event_subscribe,
> + sctp_association_event));
> +}
> diff --git a/tests/sctp/sctp_common.h b/tests/sctp/sctp_common.h
> index d5c1397..351ee37 100644
> --- a/tests/sctp/sctp_common.h
> +++ b/tests/sctp/sctp_common.h
> @@ -25,3 +25,4 @@ void print_context(int fd, char *text);
> void print_addr_info(struct sockaddr *sin, char *text);
> char *get_ip_option(int fd, bool ipv4, socklen_t *opt_len);
> void print_ip_option(int fd, bool ipv4, char *text);
> +int set_subscr_events(int fd, int data_io, int association);
> diff --git a/tests/sctp/sctp_peeloff_server.c
> b/tests/sctp/sctp_peeloff_server.c
> index 4a5110a..093c6c0 100644
> --- a/tests/sctp/sctp_peeloff_server.c
> +++ b/tests/sctp/sctp_peeloff_server.c
> @@ -16,24 +16,6 @@ static void usage(char *progname)
> exit(1);
> }
>
> -static void set_subscr_events(int fd, int value)
> -{
> - int result;
> - struct sctp_event_subscribe subscr_events;
> -
> - memset(&subscr_events, 0, sizeof(subscr_events));
> - subscr_events.sctp_association_event = value;
> - /* subscr_events.sctp_data_io_event = value; */
> -
> - result = setsockopt(fd, IPPROTO_SCTP, SCTP_EVENTS,
> - &subscr_events, sizeof(subscr_events));
> - if (result < 0) {
> - perror("Server setsockopt: SCTP_EVENTS");
> - close(fd);
> - exit(1);
> - }
> -}
> -
> static sctp_assoc_t handle_event(void *buf)
> {
> union sctp_notification *snp = buf;
> @@ -166,7 +148,13 @@ int main(int argc, char **argv)
> }
>
> do {
> - set_subscr_events(sock, 1); /* Get assoc_id for
> sctp_peeloff() */
> + /* Get assoc_id for sctp_peeloff() */
> + result = set_subscr_events(sock, 0, 1);
> + if (result < 0) {
> + perror("Server setsockopt: SCTP_EVENTS");
> + close(sock);
> + exit(1);
> + }
> sinlen = sizeof(sin);
> flags = 0;
>
> @@ -192,7 +180,7 @@ int main(int argc, char **argv)
> exit(1);
> }
> /* No more notifications */
> - set_subscr_events(sock, 0);
> + set_subscr_events(sock, 0, 0);
>
> peeloff_sk = sctp_peeloff(sock, assoc_id);
> if (peeloff_sk < 0) {
> diff --git a/tests/sctp/sctp_server.c b/tests/sctp/sctp_server.c
> index 4659ed1..7f2cd20 100644
> --- a/tests/sctp/sctp_server.c
> +++ b/tests/sctp/sctp_server.c
> @@ -134,7 +134,7 @@ int main(int argc, char **argv)
> }
>
> /* Enables sctp_data_io_events for sctp_recvmsg(3) for
> assoc_id. */
> - result = setsockopt(sock, SOL_SCTP, SCTP_EVENTS, &on,
> sizeof(on));
> + result = set_subscr_events(sock, on, 0);
> if (result < 0) {
> perror("Server setsockopt: SCTP_EVENTS");
> close(sock);
