mikaku / Fiwix

A UNIX-like kernel for the i386 architecture
https://www.fiwix.org
Other
401 stars 32 forks source link

Check diff_dev variable before accessing tmp_inode #61

Closed rick-masters closed 7 months ago

rick-masters commented 7 months ago

The following code is problematic:

https://github.com/mikaku/Fiwix/blob/74753abe61fb8e48c6748706d480b627f63ea87e/kernel/syscalls/getcwd.c#L99-L106

Consider the situation in which diff_dev is zero (false). In this case, tmp_ino will NOT be set by parse_namei (nor any other previous code). However, in the if statement at the bottom, tmp_ino is accessed before checking diff_dev and in the case where diff_dev is zero, tmp_ino will be unitialized, resulting in a possible fault.

This issue causes page faults failures in live-bootstrap like so:

 +> tcc -c putenv_stub.c                                                    
 +> tcc -static -o /usr/bin/make getopt.o getopt1.o ar.o arscan.o commands.o default.o dir.o expand.o file.o function.o implicit.o job.o main.o misc.o  
 +> make --version                                                          

Page Fault at 0x000002b6 (reading) with error code 0x00000000 (0b0)         

Note that the specific scenario in which this occurs is not consistent due to timing issues and is difficult to create a simple test case for. It is also possible that gcc might optimize this code to check diff_dev first but tcc may not perform the same optimization. So, unfortunately I can only provide a PR which fixes the issue but I don't have a test case other than running live-bootstrap.

There is obviously no downside to checking diff_dev first before checking the other criteria. A && B->C is always the same as B->C && A and can only be safer so this improvement should be self-evident.

mikaku commented 7 months ago

I guess I've not experienced those page faults because Newlib C uses its own version of getcwd(), and so it don't needs to call to sys_getcwd().

In any case, this is a great catch Rick! Thank you very much.