Memory overwrite regression in sys_getcwd

[rick-masters]

The recent change 58ca771f135779113f5181f36fea353909d92425 on March 22 changed the memory allocated for dirent_buff but did not change the size (PAGE_SIZE) passed to readdir later in the function and so readdir can read past the allocated memory. This results in a page fault when testing live-bootstrap.

You should be able to reproduce it by using the branch I provided in #27 and just applying 58ca771f to it.

The patch provide below fixes it. However, given that the buffer is being used to read multiple entries I think it might be better to also revert 58ca771f because using a larger buffer is probably more efficient.

diff --git a/kernel/syscalls/getcwd.c b/kernel/syscalls/getcwd.c
index 75838f4..3f31403 100644
--- a/kernel/syscalls/getcwd.c
+++ b/kernel/syscalls/getcwd.c
@@ -80,7 +80,7 @@ int sys_getcwd(char *buf, __size_t size)
                }
                do {
                        done = 0;
-                       bytes_read = up->fsop->readdir(up, &fd_table[tmp_fd], dirent_buf, PAGE_SIZE);
+                       bytes_read = up->fsop->readdir(up, &fd_table[tmp_fd], dirent_buf, sizeof(dirent_buf));
                        if(bytes_read < 0) {
                                release_fd(tmp_fd);
                                iput(up);

[rick-masters]

Sorry, my testing of the patch I provided was incorrect. The patch does not fix it so I'm not sure what's going on.

edit: so dirent_buf is a pointer so the patch should be sizeof(struct dirent). I'm testing that now.

[mikaku]

The recent change https://github.com/mikaku/Fiwix/commit/58ca771f135779113f5181f36fea353909d92425 on March 22 changed the memory allocated for dirent_buff but did not change the size (PAGE_SIZE) passed to readdir later in the function and so readdir can read past the allocated memory. This results in a page fault when testing live-bootstrap.

You're right. I forgot to also change the PAGE_SIZE later in the function. I was unaware of that problem because Newlib already comes with its own getcwd() function and so it doesn't call the kernel system call.

The patch provide below fixes it. However, given that the buffer is being used to read multiple entries I think it might be better to also revert https://github.com/mikaku/Fiwix/commit/58ca771f135779113f5181f36fea353909d92425 because using a larger buffer is probably more efficient.

I think you're also right here. It seems more efficient to let use a PAGE_SIZE.

I'll wait for your results and then we can decide the best approach.

[mikaku]

After your confirmation in IRC #bootstrappable, I proceed to revert https://github.com/mikaku/Fiwix/commit/58ca771f135779113f5181f36fea353909d92425.

[mikaku]

Thank you.