| Message ID | 20230923233346.12726-1-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/x86/lam: Zero out buffer for readlink() | expand |
On Sun, Sep 24, 2023 at 07:33:46AM +0800, Binbin Wu wrote: > Zero out the buffer for readlink() since readlink() does not append a > terminating null byte to the buffer. > > Fixes: 833c12ce0f430 ("selftests/x86/lam: Add inherit test cases for linear-address masking") > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > tools/testing/selftests/x86/lam.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c > index eb0e46905bf9..9f06942a8e25 100644 > --- a/tools/testing/selftests/x86/lam.c > +++ b/tools/testing/selftests/x86/lam.c > @@ -680,7 +680,7 @@ static int handle_execve(struct testcases *test) > perror("Fork failed."); > ret = 1; > } else if (pid == 0) { > - char path[PATH_MAX]; > + char path[PATH_MAX] = {0}; Shouldn't it be PATH_MAX+1 to handle the case when readlink(2) stores exactly PATH_MAX bytes into the buffer? > > /* Set LAM mode in parent process */ > if (set_lam(lam) != 0) > > base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70 > -- > 2.25.1 >
On 9/27/2023 7:02 PM, kirill.shutemov@linux.intel.com wrote: > On Sun, Sep 24, 2023 at 07:33:46AM +0800, Binbin Wu wrote: >> Zero out the buffer for readlink() since readlink() does not append a >> terminating null byte to the buffer. >> >> Fixes: 833c12ce0f430 ("selftests/x86/lam: Add inherit test cases for linear-address masking") >> >> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >> --- >> tools/testing/selftests/x86/lam.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c >> index eb0e46905bf9..9f06942a8e25 100644 >> --- a/tools/testing/selftests/x86/lam.c >> +++ b/tools/testing/selftests/x86/lam.c >> @@ -680,7 +680,7 @@ static int handle_execve(struct testcases *test) >> perror("Fork failed."); >> ret = 1; >> } else if (pid == 0) { >> - char path[PATH_MAX]; >> + char path[PATH_MAX] = {0}; > Shouldn't it be PATH_MAX+1 to handle the case when readlink(2) stores > exactly PATH_MAX bytes into the buffer? According to the definition of PATH_MAX in include/uapi/linux/limits.h #define PATH_MAX 4096 /* # chars in a path name including nul */ IIUC, Linux limits the path length to 4095 and PATH_MAX includes the terminating nul. > >> >> /* Set LAM mode in parent process */ >> if (set_lam(lam) != 0) >> >> base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70 >> -- >> 2.25.1 >>
On Tue, Oct 10, 2023 at 11:51:32AM +0800, Binbin Wu wrote: > > > On 9/27/2023 7:02 PM, kirill.shutemov@linux.intel.com wrote: > > On Sun, Sep 24, 2023 at 07:33:46AM +0800, Binbin Wu wrote: > > > Zero out the buffer for readlink() since readlink() does not append a > > > terminating null byte to the buffer. > > > > > > Fixes: 833c12ce0f430 ("selftests/x86/lam: Add inherit test cases for linear-address masking") > > > > > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > > > --- > > > tools/testing/selftests/x86/lam.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c > > > index eb0e46905bf9..9f06942a8e25 100644 > > > --- a/tools/testing/selftests/x86/lam.c > > > +++ b/tools/testing/selftests/x86/lam.c > > > @@ -680,7 +680,7 @@ static int handle_execve(struct testcases *test) > > > perror("Fork failed."); > > > ret = 1; > > > } else if (pid == 0) { > > > - char path[PATH_MAX]; > > > + char path[PATH_MAX] = {0}; > > Shouldn't it be PATH_MAX+1 to handle the case when readlink(2) stores > > exactly PATH_MAX bytes into the buffer? > According to the definition of PATH_MAX in include/uapi/linux/limits.h > #define PATH_MAX 4096 /* # chars in a path name including nul */ > > IIUC, Linux limits the path length to 4095 and PATH_MAX includes the > terminating nul. Consider the case when kernel bump PATH_MAX to 8192. The binary that compiled from lam.c against the older kernel headers will get compromised. Increase the size of the buffer by one or pass PATH_MAX - 1 as buffer size to readlink(2).
On 10/10/2023 1:46 PM, kirill.shutemov@linux.intel.com wrote: > On Tue, Oct 10, 2023 at 11:51:32AM +0800, Binbin Wu wrote: >> >> On 9/27/2023 7:02 PM, kirill.shutemov@linux.intel.com wrote: >>> On Sun, Sep 24, 2023 at 07:33:46AM +0800, Binbin Wu wrote: >>>> Zero out the buffer for readlink() since readlink() does not append a >>>> terminating null byte to the buffer. >>>> >>>> Fixes: 833c12ce0f430 ("selftests/x86/lam: Add inherit test cases for linear-address masking") >>>> >>>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >>>> --- >>>> tools/testing/selftests/x86/lam.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c >>>> index eb0e46905bf9..9f06942a8e25 100644 >>>> --- a/tools/testing/selftests/x86/lam.c >>>> +++ b/tools/testing/selftests/x86/lam.c >>>> @@ -680,7 +680,7 @@ static int handle_execve(struct testcases *test) >>>> perror("Fork failed."); >>>> ret = 1; >>>> } else if (pid == 0) { >>>> - char path[PATH_MAX]; >>>> + char path[PATH_MAX] = {0}; >>> Shouldn't it be PATH_MAX+1 to handle the case when readlink(2) stores >>> exactly PATH_MAX bytes into the buffer? >> According to the definition of PATH_MAX in include/uapi/linux/limits.h >> #define PATH_MAX 4096 /* # chars in a path name including nul */ >> >> IIUC, Linux limits the path length to 4095 and PATH_MAX includes the >> terminating nul. > Consider the case when kernel bump PATH_MAX to 8192. The binary that > compiled from lam.c against the older kernel headers will get compromised. > > Increase the size of the buffer by one or pass PATH_MAX - 1 as buffer size > to readlink(2). Make sense, thanks! I will send a new version as following: diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index eb0e46905bf9..8f9b06d9ce03 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -573,7 +573,7 @@ int do_uring(unsigned long lam) char path[PATH_MAX] = {0}; /* get current process path */ - if (readlink("/proc/self/exe", path, PATH_MAX) <= 0) + if (readlink("/proc/self/exe", path, PATH_MAX - 1) <= 0) return 1; int file_fd = open(path, O_RDONLY); @@ -680,14 +680,14 @@ static int handle_execve(struct testcases *test) perror("Fork failed."); ret = 1; } else if (pid == 0) { - char path[PATH_MAX]; + char path[PATH_MAX] = {0}; /* Set LAM mode in parent process */ if (set_lam(lam) != 0) return 1; /* Get current binary's path and the binary was run by execve */ - if (readlink("/proc/self/exe", path, PATH_MAX) <= 0) + if (readlink("/proc/self/exe", path, PATH_MAX - 1) <= 0) exit(-1); /* run binary to get LAM mode and return to parent process */ >
diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index eb0e46905bf9..9f06942a8e25 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -680,7 +680,7 @@ static int handle_execve(struct testcases *test) perror("Fork failed."); ret = 1; } else if (pid == 0) { - char path[PATH_MAX]; + char path[PATH_MAX] = {0}; /* Set LAM mode in parent process */ if (set_lam(lam) != 0)
Zero out the buffer for readlink() since readlink() does not append a terminating null byte to the buffer. Fixes: 833c12ce0f430 ("selftests/x86/lam: Add inherit test cases for linear-address masking") Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> --- tools/testing/selftests/x86/lam.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70