| Message ID | 20200224124710.156385-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [testsuite] tests/sctp: fix setting of the SCTP_EVENTS sockopt | expand |
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@redhat.com> > --- > 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);
On Mon, Feb 24, 2020 at 4:24 PM Richard Haines <richard_c_haines@btinternet.com> 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@mail.gmail.com/T/ > > > > > Fixes: c38b57ffdac4 ("selinux-testsuite: Add SCTP test support") > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > 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); >
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);
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. Fixes: c38b57ffdac4 ("selinux-testsuite: Add SCTP test support") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- 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(-)