On Mon, Feb 24, 2020 at 4:24 PM Richard Haines <richard_c_haines@xxxxxxxxxxxxxx> wrote: > 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 ? Apparently, struct sctp_event_subscribe has been extended by a new field between kernel 5.4 and 5.5 [1] and this caused the error to appear in CKI testing [2] when Fedora 31 moved to kernel 5.5 (it installed the tested 5.4 stable rc kernel and booted into it, while the kernel-headers package stayed at the distro's 5.5 version). The endianness issue never seemed to have triggered actual failures when we ran the tests internally on big-endian s390x systems. (I'm wondering if the SCTP_EVENTS setsockopt() is even needed there...?) [1] https://github.com/torvalds/linux/blame/master/include/uapi/linux/sctp.h#L623 [2] https://lore.kernel.org/stable/CAFqZXNvnywGVwqJhXcfwpmx2mmu8ajAUOdy0Ny8PvsT=Rg_3Qg@xxxxxxxxxxxxxx/T/ > > > > > 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); > -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
