| Message ID | 20200225094529.178623-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [testsuite,v2] tests/sctp: fix setting of the SCTP_EVENTS sockopt | expand |
On Tue, 2020-02-25 at 10:45 +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. > > Fixes: c38b57ffdac4 ("selinux-testsuite: Add SCTP test support") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Just tested this with no problems. Thanks for the fixes. Acked-by: Richard Haines <richard_c_haines@btinternet.com> > --- > > v2: check the result of second set_subscr_events() call in > sctp_peeloff_server.c > > tests/sctp/sctp_common.c | 20 +++++++++++++++++++ > tests/sctp/sctp_common.h | 1 + > tests/sctp/sctp_peeloff_server.c | 33 +++++++++++++----------------- > -- > tests/sctp/sctp_server.c | 2 +- > 4 files changed, 35 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..8350cb4 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,12 @@ int main(int argc, char **argv) > exit(1); > } > /* No more notifications */ > - set_subscr_events(sock, 0); > + result = set_subscr_events(sock, 0, 0); > + if (result < 0) { > + perror("Server setsockopt: > SCTP_EVENTS"); > + close(sock); > + exit(1); > + } > > 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 Tue, Feb 25, 2020 at 4:51 AM Ondrej Mosnacek <omosnace@redhat.com> 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. > > Fixes: c38b57ffdac4 ("selinux-testsuite: Add SCTP test support") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
On Fri, Feb 28, 2020 at 8:18 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Feb 25, 2020 at 4:51 AM Ondrej Mosnacek <omosnace@redhat.com> 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. > > > > Fixes: c38b57ffdac4 ("selinux-testsuite: Add SCTP test support") > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> And applied. BTW this was my first time using get-lore-mbox.py [1] and I really liked the automatic collection of acks into the commit message. [1] https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/get-lore-mbox.py
On February 28, 2020 2:23:27 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Fri, Feb 28, 2020 at 8:18 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: >> >> On Tue, Feb 25, 2020 at 4:51 AM Ondrej Mosnacek <omosnace@redhat.com> 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. >>> >>> Fixes: c38b57ffdac4 ("selinux-testsuite: Add SCTP test support") >>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> >> >> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > And applied. BTW this was my first time using get-lore-mbox.py [1] > and I really liked the automatic collection of acks into the commit > message. > [1] https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/get-lore-mbox.py FYI, the kernel.org patchwork instance collects the ACKs/Reviews if you download the mbox file. That said, there has been a lot of work going into the lore scripts lately and I expect there to be a lot more development in that area in the future. -- paul moore www.paul-moore.com
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..8350cb4 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,12 @@ int main(int argc, char **argv) exit(1); } /* No more notifications */ - set_subscr_events(sock, 0); + result = set_subscr_events(sock, 0, 0); + if (result < 0) { + perror("Server setsockopt: SCTP_EVENTS"); + close(sock); + exit(1); + } 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> --- v2: check the result of second set_subscr_events() call in sctp_peeloff_server.c tests/sctp/sctp_common.c | 20 +++++++++++++++++++ tests/sctp/sctp_common.h | 1 + tests/sctp/sctp_peeloff_server.c | 33 +++++++++++++------------------- tests/sctp/sctp_server.c | 2 +- 4 files changed, 35 insertions(+), 21 deletions(-)