| Message ID | 20240718164842.3650702-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/libxs: Fix SIGPIPE handling issues | expand |
On 2024-07-18 12:48, Andrew Cooper wrote: > If the sum of iov element lengths overflows, the XENSTORE_PAYLOAD_MAX can > pass, after which we'll write 4G of data with a good-looking length field, and > the remainder of the payload will be interpreted as subsequent commands. > > Check each iov element length for XENSTORE_PAYLOAD_MAX before accmulating it. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Anthony PERARD <anthony.perard@vates.tech> > CC: Juergen Gross <jgross@suse.com> > --- > tools/libs/store/xs.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c > index ec77379ab9bd..81a790cfe60f 100644 > --- a/tools/libs/store/xs.c > +++ b/tools/libs/store/xs.c > @@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h, xs_transaction_t t, > struct xsd_sockmsg msg; > void *ret = NULL; > int saved_errno; > - unsigned int i; > + unsigned int i, msg_len; > struct sigaction ignorepipe, oldact; > > msg.tx_id = t; > msg.req_id = 0; > msg.type = type; > - msg.len = 0; > - for (i = 0; i < num_vecs; i++) > - msg.len += iovec[i].iov_len; > > - if (msg.len > XENSTORE_PAYLOAD_MAX) { > - errno = E2BIG; > - return 0; > + /* Calculate the payload length by summing iovec elements */ > + for (i = 0, msg_len = 0; i < num_vecs; i++) { > + if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) || > + ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) { > + errno = E2BIG; > + return 0; return NULL; With that, Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
On 19.07.24 23:14, Jason Andryuk wrote: > On 2024-07-18 12:48, Andrew Cooper wrote: >> If the sum of iov element lengths overflows, the XENSTORE_PAYLOAD_MAX can >> pass, after which we'll write 4G of data with a good-looking length field, and >> the remainder of the payload will be interpreted as subsequent commands. >> >> Check each iov element length for XENSTORE_PAYLOAD_MAX before accmulating it. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Anthony PERARD <anthony.perard@vates.tech> >> CC: Juergen Gross <jgross@suse.com> >> --- >> tools/libs/store/xs.c | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c >> index ec77379ab9bd..81a790cfe60f 100644 >> --- a/tools/libs/store/xs.c >> +++ b/tools/libs/store/xs.c >> @@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h, >> xs_transaction_t t, >> struct xsd_sockmsg msg; >> void *ret = NULL; >> int saved_errno; >> - unsigned int i; >> + unsigned int i, msg_len; >> struct sigaction ignorepipe, oldact; >> msg.tx_id = t; >> msg.req_id = 0; >> msg.type = type; >> - msg.len = 0; >> - for (i = 0; i < num_vecs; i++) >> - msg.len += iovec[i].iov_len; >> - if (msg.len > XENSTORE_PAYLOAD_MAX) { >> - errno = E2BIG; >> - return 0; >> + /* Calculate the payload length by summing iovec elements */ >> + for (i = 0, msg_len = 0; i < num_vecs; i++) { >> + if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) || >> + ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) { >> + errno = E2BIG; >> + return 0; > > return NULL; > > With that, > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> With the suggested change: Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
On 22/07/2024 10:19 am, Jürgen Groß wrote: > On 19.07.24 23:14, Jason Andryuk wrote: >> On 2024-07-18 12:48, Andrew Cooper wrote: >>> If the sum of iov element lengths overflows, the >>> XENSTORE_PAYLOAD_MAX can >>> pass, after which we'll write 4G of data with a good-looking length >>> field, and >>> the remainder of the payload will be interpreted as subsequent >>> commands. >>> >>> Check each iov element length for XENSTORE_PAYLOAD_MAX before >>> accmulating it. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Anthony PERARD <anthony.perard@vates.tech> >>> CC: Juergen Gross <jgross@suse.com> >>> --- >>> tools/libs/store/xs.c | 17 ++++++++++------- >>> 1 file changed, 10 insertions(+), 7 deletions(-) >>> >>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c >>> index ec77379ab9bd..81a790cfe60f 100644 >>> --- a/tools/libs/store/xs.c >>> +++ b/tools/libs/store/xs.c >>> @@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h, >>> xs_transaction_t t, >>> struct xsd_sockmsg msg; >>> void *ret = NULL; >>> int saved_errno; >>> - unsigned int i; >>> + unsigned int i, msg_len; >>> struct sigaction ignorepipe, oldact; >>> msg.tx_id = t; >>> msg.req_id = 0; >>> msg.type = type; >>> - msg.len = 0; >>> - for (i = 0; i < num_vecs; i++) >>> - msg.len += iovec[i].iov_len; >>> - if (msg.len > XENSTORE_PAYLOAD_MAX) { >>> - errno = E2BIG; >>> - return 0; >>> + /* Calculate the payload length by summing iovec elements */ >>> + for (i = 0, msg_len = 0; i < num_vecs; i++) { >>> + if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) || >>> + ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) { >>> + errno = E2BIG; >>> + return 0; >> >> return NULL; >> >> With that, >> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> > > With the suggested change: > > Reviewed-by: Juergen Gross <jgross@suse.com> Thankyou both. I'd not even spotted the mistake when just rearranging the logic. ~Andrew
diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c index ec77379ab9bd..81a790cfe60f 100644 --- a/tools/libs/store/xs.c +++ b/tools/libs/store/xs.c @@ -571,21 +571,24 @@ static void *xs_talkv(struct xs_handle *h, xs_transaction_t t, struct xsd_sockmsg msg; void *ret = NULL; int saved_errno; - unsigned int i; + unsigned int i, msg_len; struct sigaction ignorepipe, oldact; msg.tx_id = t; msg.req_id = 0; msg.type = type; - msg.len = 0; - for (i = 0; i < num_vecs; i++) - msg.len += iovec[i].iov_len; - if (msg.len > XENSTORE_PAYLOAD_MAX) { - errno = E2BIG; - return 0; + /* Calculate the payload length by summing iovec elements */ + for (i = 0, msg_len = 0; i < num_vecs; i++) { + if ((iovec[i].iov_len > XENSTORE_PAYLOAD_MAX) || + ((msg_len += iovec[i].iov_len) > XENSTORE_PAYLOAD_MAX)) { + errno = E2BIG; + return 0; + } } + msg.len = msg_len; + ignorepipe.sa_handler = SIG_IGN; sigemptyset(&ignorepipe.sa_mask); ignorepipe.sa_flags = 0;
If the sum of iov element lengths overflows, the XENSTORE_PAYLOAD_MAX can pass, after which we'll write 4G of data with a good-looking length field, and the remainder of the payload will be interpreted as subsequent commands. Check each iov element length for XENSTORE_PAYLOAD_MAX before accmulating it. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Anthony PERARD <anthony.perard@vates.tech> CC: Juergen Gross <jgross@suse.com> --- tools/libs/store/xs.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)