diff --git a/.github/workflows/fileio-reply-tests.yml b/.github/workflows/fileio-reply-tests.yml new file mode 100644 index 0000000..8362d94 --- /dev/null +++ b/.github/workflows/fileio-reply-tests.yml @@ -0,0 +1,16 @@ +name: IOP File-I/O Reply Tests + +on: + push: + pull_request: + workflow_dispatch: {} + +jobs: + fileio-reply-test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Validate TCP reply types, bounds and socket invalidation (plain C) + run: | + cc -std=c99 -Wall -Wextra -Werror -Iinclude tests/fileio_reply.c -o /tmp/ps2link-fileio-reply-test + /tmp/ps2link-fileio-reply-test diff --git a/iop/fileio_reply.h b/iop/fileio_reply.h new file mode 100644 index 0000000..1c01c85 --- /dev/null +++ b/iop/fileio_reply.h @@ -0,0 +1,37 @@ +#ifndef PS2LINK_FILEIO_REPLY_H +#define PS2LINK_FILEIO_REPLY_H + +#include "hostlink.h" + +#define PKO_FILEIO_PACKET_LIMIT 4096 + +/* Detach before disconnect, so every close path invalidates the handle. */ +static int pko_fileio_detach_socket(int *current_socket) +{ + int socket = *current_socket; + *current_socket = -1; + return socket; +} + +static int pko_fileio_reply_header_valid(unsigned int command, + unsigned int length, + unsigned int expected_command, + unsigned int expected_length) +{ + return expected_length >= sizeof(pko_pkt_hdr) && + expected_length <= PKO_FILEIO_PACKET_LIMIT && + command == expected_command && length == expected_length; +} + +static int pko_fileio_read_length_valid(int received, int requested) +{ + return requested >= 0 && received >= 0 && received <= requested; +} + +/* A successful nonempty write must advance by no more than it requested. */ +static int pko_fileio_write_progress_valid(int written, int requested) +{ + return requested > 0 && written > 0 && written <= requested; +} + +#endif diff --git a/iop/net_fio.c b/iop/net_fio.c index a553c2a..b7ed26a 100644 --- a/iop/net_fio.c +++ b/iop/net_fio.c @@ -18,12 +18,12 @@ #include "ps2ip.h" #include "net_fio.h" -#include "hostlink.h" +#include "fileio_reply.h" #include "globals.h" unsigned int remote_pc_addr = 0xffffffff; -#define PACKET_MAXSIZE 4096 +#define PACKET_MAXSIZE PKO_FILEIO_PACKET_LIMIT static char send_packet[PACKET_MAXSIZE] __attribute__((aligned(16))); static char recv_packet[PACKET_MAXSIZE] __attribute__((aligned(16))); @@ -36,25 +36,22 @@ static int pko_fileio_active = 0; void pko_close_socket(void) { int ret; + int sock = pko_fileio_detach_socket(&pko_fileio_sock); - if (pko_fileio_sock < 0) + if (sock < 0) return; - ret = disconnect(pko_fileio_sock); + ret = disconnect(sock); if (ret < 0) { printf("pko_file: disconnect returned error %d\n", ret); } - pko_fileio_sock = -1; } //---------------------------------------------------------------------- // void pko_close_fsys(void) { - if (pko_fileio_sock > 0) { - disconnect(pko_fileio_sock); - } pko_fileio_active = 0; - return; + pko_close_socket(); } //---------------------------------------------------------------------- @@ -109,6 +106,12 @@ int pko_accept_pkt(int sock, char *buf, int len, int pkt_type) unsigned short hlen; + if (len < (int)sizeof(pko_pkt_hdr) || len > PACKET_MAXSIZE) { + dbgprintf("pko_file: invalid expected reply size %d\n", len); + pko_close_socket(); + return 0; + } + length = pko_recv_bytes(sock, buf, sizeof(pko_pkt_hdr)); if (length < 0) { dbgprintf("pko_file: accept_pkt recv error\n"); @@ -128,32 +131,20 @@ int pko_accept_pkt(int sock, char *buf, int len, int pkt_type) if (hcmd != pkt_type) { dbgprintf("pko_file: pko_accept_pkt: Expected %x, got %x\n", pkt_type, hcmd); - return 0; - } - - if (hlen != len || hlen < sizeof(pko_pkt_hdr)) { + } else if (!pko_fileio_reply_header_valid(hcmd, hlen, pkt_type, len)) { dbgprintf("pko_file: pko_accept_pkt: unexpected packet length " "(%d, expected %d)\n", hlen, len); - return 0; - } - - // get the actual packet data - length = pko_recv_bytes(sock, buf + sizeof(pko_pkt_hdr), - hlen - sizeof(pko_pkt_hdr)); - - if (length < 0) { - dbgprintf("pko_file: accept recv2 error!!\n"); - return 0; + } else { + /* Only a fully validated reply may remain on the TCP stream. */ + length = pko_recv_bytes(sock, buf + sizeof(pko_pkt_hdr), + hlen - sizeof(pko_pkt_hdr)); + return length == (int)(hlen - sizeof(pko_pkt_hdr)); } - if (length < (hlen - sizeof(pko_pkt_hdr))) { - dbgprintf("pko_accept_pkt: Did not receive full packet!!! " - "Fix this! (%d)\n", - length); - } - - return 1; + /* The remaining stream cannot be resynchronized after a bad header. */ + pko_close_socket(); + return 0; } //---------------------------------------------------------------------- @@ -284,7 +275,7 @@ int pko_write_file(int fd, char *buf, int length) int nbytes; int retval; - if (pko_fileio_sock < 0) { + if (pko_fileio_sock < 0 || length < 0) { return -1; } @@ -346,6 +337,13 @@ int pko_write_file(int fd, char *buf, int length) return retval; } + if (!pko_fileio_write_progress_valid(retval, nbytes)) { + dbgprintf("pko_file: invalid write reply %d (requested %d)\n", + retval, nbytes); + pko_close_socket(); + return -1; + } + writtenbytes += retval; if (retval < nbytes) { // EOF? @@ -403,9 +401,10 @@ int pko_read_file(int fd, char *buf, int length) "(wanted %d)\n", nbytes, length); - if (nbytes < 0 || nbytes > length) { + if (!pko_fileio_read_length_valid(nbytes, length)) { dbgprintf("pko_file: invalid read length %d (requested %d)\n", nbytes, length); + pko_close_socket(); return -1; } @@ -758,10 +757,9 @@ int pko_file_serv(void *argv) remote_pc_addr = client_addr.sin_addr.s_addr; - if (pko_fileio_sock > 0) { + if (pko_fileio_sock >= 0) { dbgprintf("Client reconnected\n"); - ret = disconnect(pko_fileio_sock); - dbgprintf("close ret %d\n", ret); + pko_close_socket(); } /* Disable Nagle on IOP side so small request packets are sent @@ -773,9 +771,7 @@ int pko_file_serv(void *argv) pko_fileio_sock = client_sock; } - if (pko_fileio_sock > 0) { - disconnect(pko_fileio_sock); - } + pko_close_socket(); disconnect(sock); diff --git a/tests/fileio_reply.c b/tests/fileio_reply.c new file mode 100644 index 0000000..b14a0b9 --- /dev/null +++ b/tests/fileio_reply.c @@ -0,0 +1,57 @@ +#include +#include "../iop/fileio_reply.h" + +static int failed; + +static void check(int ok, const char *case_name) +{ + if (!ok) { + fprintf(stderr, "FAIL %s\n", case_name); + failed++; + } +} + +int main(void) +{ + unsigned int len = sizeof(pko_pkt_file_rly); + int descriptor = 0; + + check(pko_fileio_detach_socket(&descriptor) == 0 && descriptor == -1, + "socket zero is valid and invalidated on close"); + descriptor = 17; + check(pko_fileio_detach_socket(&descriptor) == 17 && descriptor == -1, + "connected socket is invalidated on close"); + check(pko_fileio_detach_socket(&descriptor) == -1 && descriptor == -1, + "closing an invalid socket is idempotent"); + + check(pko_fileio_reply_header_valid(PKO_WRITE_RLY, len, PKO_WRITE_RLY, len), + "valid write reply header"); + check(!pko_fileio_reply_header_valid(PKO_READ_RLY, len, PKO_WRITE_RLY, len), + "mismatched reply type"); + check(!pko_fileio_reply_header_valid(PKO_WRITE_RLY, len + 1, PKO_WRITE_RLY, len), + "oversized reply"); + check(!pko_fileio_reply_header_valid(PKO_WRITE_RLY, len - 1, PKO_WRITE_RLY, len), + "truncated reply"); + check(!pko_fileio_reply_header_valid(PKO_WRITE_RLY, len, PKO_WRITE_RLY, 5), + "invalid expected short packet"); + check(!pko_fileio_reply_header_valid(PKO_WRITE_RLY, len, PKO_WRITE_RLY, 4097), + "invalid expected oversized packet"); + + check(pko_fileio_read_length_valid(0, 10), "empty read"); + check(pko_fileio_read_length_valid(4, 10), "short read"); + check(pko_fileio_read_length_valid(10, 10), "full read"); + check(!pko_fileio_read_length_valid(-1, 10), "negative read"); + check(!pko_fileio_read_length_valid(11, 10), "oversized read"); + check(!pko_fileio_read_length_valid(1, -1), "negative request"); + + check(pko_fileio_write_progress_valid(4, 10), "short write"); + check(pko_fileio_write_progress_valid(10, 10), "full write"); + check(!pko_fileio_write_progress_valid(0, 10), "zero progress"); + check(!pko_fileio_write_progress_valid(-1, 10), "negative write"); + check(!pko_fileio_write_progress_valid(11, 10), "oversized write"); + check(!pko_fileio_write_progress_valid(1, 0), "zero request"); + + if (failed) return 1; + puts("PASS fileio reply headers and read/write bounds"); + return 0; +}