ps2link: Harden TCP file-I/O socket lifecycle and malformed replies

.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

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

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);
 

tests/fileio_reply.c

@@ -0,0 +1,57 @@
+#include <stdio.h>
+#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;
+}