Junbo-Zheng opened a new pull request, #20440:
URL: https://github.com/apache/nuttx/pull/20440
## Summary
`ttyname_r()` passed the caller buffer straight to `fcntl(fd, F_GETPATH,
buf)` as soon as `buflen >= TTY_NAME_MAX`. But the F_GETPATH contract is a
buffer of `PATH_MAX` or greater (`include/nuttx/fs/ioctl.h`, FIOC_FILEPATH),
and every in-tree handler writes the path bounded by `PATH_MAX`, ignoring the
caller buffer size (e.g. `fs/tmpfs/fs_tmpfs.c` and `fs/romfs/fs_romfs.c` both
call `inode_getpath(inode, ptr, PATH_MAX)`).
A tty whose path is longer than `TTY_NAME_MAX` (== `CONFIG_NAME_MAX`, 32 in
most configs) therefore overflows the caller buffer while `ttyname_r()` returns
0. This is reachable with a tty driver registered under a nested /dev path
(legal in NuttX, cf. `/dev/input/btn0`) or reached through rpmsgfs, whose
mountpoint depth is unbounded. The small-buffer branch has the same defect
against its own stack local `char name[TTY_NAME_MAX]`.
The fix gates the direct write on `PATH_MAX` instead of `TTY_NAME_MAX` and
stages the path through a `PATH_MAX`-sized local first, returning `ERANGE` when
it does not fit the caller buffer.
## Impact
- **Users**: `ttyname_r()` now returns `ERANGE` instead of silently
overflowing the caller buffer when the tty path does not fit; callers that pass
a `PATH_MAX` buffer see no change.
- **Build**: None.
- **Hardware**: None.
- **Documentation**: None.
- **Security & Compatibility**: Fixes a silent stack/TLS memory overwrite
(the `ttyname()` wrapper stores the name in a `TTY_NAME_MAX` buffer); no API
change.
## Testing
Carried a scratch regression test in `apps/examples/hello/hello_main.c`
```diff
diff --git a/examples/hello/hel/hello_main.c
--- a/examples/hello/hello_main.c
+++ b/examples/hello/hello_main.c
@@ -26,6 +26,81 @@
#include <nuttx/config.h>
#include <stdio.h>
+#include <stdint.h>
+#include <string.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <errno.h>
+#include <limits.h>
+#include <termios.h>
+
+#include <nuttx/fs/fs.h>
+#include <nuttx/fs/ioctl.h>
+
+/**************************************************
+ * Pre-processor Definitions
+ **************************************************/
+
+/* A tty device whose register TTY_NAME_MAX
+ * (== CONFIG_NAME_MAX, typically 32). Nested /dev paths of this shape
+ * are legal in NuttX (cf. /de
+ */
+
+#define LONGTTY_PATH "/dev/ttyname_r_regression/" \
+ "device_
+ "very_long_nested_name_0123456789"
+
+#define CANARY_BYTE 0xa5
+
+/****************************************************************************
+ * Private Data
+
****************************************************************************/
+
+/* Minimal character driver: TCGETS succeeds (so isatty() is true) and
+ * FIOC_FILEPATH reports the rke the real
+ * filesystem handlers (fs_romfs.c:613, fs_tmpfs.c:2029) which copy the
+ * path into the caller bufferby any tty limit.
+ */
+
+static int ttytest_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
+{
+ switch (cmd)
+ {
+ case TCGETS:
+ return 0;
+
+ case FIOC_FILEPATH:
+ strlcpy((FAR char *)(uintptr_t)arg, LONGTTY_PATH, PATH_MAX);
+ return 0;
+
+ default:
+ return -ENOTTY;
+ }
+}
+
+static const struct file_operations g_ttytest_fops =
+{
+ .ioctl = ttytest_ioctl,
+};
+
+/**************************************************
+ * Private Functions
+ **************************************************/
+
+static int canary_intact(FAR clen)
+{
+ size_t i;
+
+ for (i = 0; i < len; i++)
+ {
+ if ((unsigned char)regio
+ {
+ return 0;
+ }
+ }
+
+ return 1;
+}
/****************************************************************************
* Public Functions
@@ -37,6 +112,78 @@
int main(int argc, FAR char *argv[])
{
+ /* Guarded buffer: what a POSIX-correct caller of ttyname_r() provides
+ * (TTY_NAME_MAX bytes, per ith canaries on
+ * both sides. The post-region is PATH_MAX-sized so that an overflowing
+ * FIOC_FILEPATH write staysdetectable instead
+ * of smashing unrelated stack.
+ */
+
+ struct
+ {
+ char pre[16];
+ char buf[TTY_NAME_MAX];
+ char post[PATH_MAX];
+ } guard;
+
+ char bigbuf[PATH_MAX];
+ int fd;
+ int ret;
+
printf("Hello, World!!\n");
+
+ /* Set up the long-path tty */
+
+ ret = register_driver(LONGTTY_PATH, &g_ttytest_fops, 0666, NULL);
+ if (ret < 0)
+ {
+ printf("ttyname_r test: \n", ret);
+ return 1;
+ }
+
+ fd = open(LONGTTY_PATH, O_RD
+ if (fd < 0)
+ {
+ printf("ttyname_r test: open failed: %d\n", get_errno());
+ unregister_driver(LONGTT
+ return 1;
+ }
+
+ printf("ttyname_r test: pathsatty=%d\n",
+ sizeof(LONGTTY_PATH) - 1, TTY_NAME_MAX, isatty(fd));
+
+ /* Case 1: caller provides a PATH_MAX buffer - always correct */
+
+ memset(bigbuf, 0, sizeof(bigbuf));
+ ret = ttyname_r(fd, bigbuf,
+ printf("case1 (PATH_MAX buf): ret=%d name=\"%s\"\n", ret, bigbuf);
+
+ /* Case 2: caller provides a TTY_NAME_MAX buffer (the ttyname() case) */
+
+ memset(&guard, CANARY_BYTE, sizeof(guard));
+ memset(guard.buf, 0, sizeof(
+
+ ret = ttyname_r(fd, guard.bu
+
+ printf("case2 (TTY_NAME_MAX ",
+ ret, guard.buf);
+ printf("case2: pre-canary %
+ canary_intact(guard.pre, sizeof(guard.pre)) ? "INTACT" :
"SMASHED");
+ printf("case2: post-canary %
+ canary_intact(guard.post, sizeof(guard.post)) ? "INTACT" :
"SMASHED");
+
+ /* Case 3: caller provides a buffer smaller than TTY_NAME_MAX */
+
+ printf("case3 (small buf): calling ttyname_r, stack may be smashed "
+ "next...\n");
+ fflush(stdout);
+
+ ret = ttyname_r(fd, bigbuf, 8);
+
+ printf("case3: survived, ret=%d errno=%d\n", ret, get_errno());
+
+ close(fd);
+ unregister_driver(LONGTTY_PA
+ printf("ttyname_r test: done\n");
return 0;
}
```
Built and run on the sim host
```
cmake -B build-sim -DBOARD_
cmake --build build-sim -j
./build-sim/nuttx
```
Before the fix:
```
ttyname_r test: path len=85 TTY_NAME_MAX=32 isatty=1
case1 (PATH_MAX buf): ret=0
name="/dev/ttyname_r_regression/device_with_a_deliberately/very_long_nested_name_0123456789"
case2 (TTY_NAME_MAX buf): ret=0
name="/dev/ttyname_r_regression/device_with_a_deliberately/very_long_nested_name_0123456789"
case2: pre-canary INTACT
case2: post-canary SMASHED
case3 (small buf): calling ttyname_r, stack may be smashed next...
dump_assert_info: Assertion failed panic: at file: :0 task: hello
process: hello
```
(case 2 silently corrupted the ffer with a zero return code; case 3
panicked.)
After the fix:
```
ttyname_r test: path len=85 TTY_NAME_MAX=32 isatty=1
case1 (PATH_MAX buf): ret=0
name="/dev/ttyname_r_regression/device_with_a_deliberately/very_long_nested_name_0123456789"
case2 (TTY_NAME_MAX buf): ret=34 name=""
case2: pre-canary INTACT
case2: post-canary INTACT
case3 (small buf): calling ttyname_r, stack may be smashed next...
case3: survived, ret=34 errno=0
ttyname_r test: done
```
(34 == ERANGE; both canaries intact, no crash, and the PATH_MAX fast path is
unchanged.)
Signed-off-by: Junbo Zheng <[email protected]>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
