target/ppc/kvm: Skip ".." directory in kvmppc_find_cpu_dt

[PATCH] target/ppc/kvm: Skip ".." directory in kvmppc_find_cpu_dt

Posted by Murilo Opsfelder Araujo 3 years, 7 months ago

Some systems have /proc/device-tree/cpus/../clock-frequency. However,
this is not the expected path for a CPU device tree directory.

Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/kvm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 6eed466f80..c8485a5cc0 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1877,6 +1877,12 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
     buf[0] = '\0';
     while ((dirp = readdir(dp)) != NULL) {
         FILE *f;
+
+        /* Don't accidentally read from the upper directory */
+        if (strcmp(dirp->d_name, "..") == 0) {
+            continue;
+        }
+
         snprintf(buf, buf_len, "%s%s/clock-frequency", PROC_DEVTREE_CPU,
                  dirp->d_name);
         f = fopen(buf, "r");
-- 
2.36.1

Re: [PATCH] target/ppc/kvm: Skip ".." directory in kvmppc_find_cpu_dt

Posted by David Gibson 3 years, 7 months ago

On Mon, Jul 11, 2022 at 04:37:43PM -0300, Murilo Opsfelder Araujo wrote:
> Some systems have /proc/device-tree/cpus/../clock-frequency. However,
> this is not the expected path for a CPU device tree directory.
> 
> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  target/ppc/kvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 6eed466f80..c8485a5cc0 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1877,6 +1877,12 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
>      buf[0] = '\0';
>      while ((dirp = readdir(dp)) != NULL) {
>          FILE *f;
> +
> +        /* Don't accidentally read from the upper directory */
> +        if (strcmp(dirp->d_name, "..") == 0) {

It might not be causing problems now, but it would be technically more
correct to also skip ".", wouldn't it?

> +            continue;
> +        }
> +
>          snprintf(buf, buf_len, "%s%s/clock-frequency", PROC_DEVTREE_CPU,
>                   dirp->d_name);
>          f = fopen(buf, "r");

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] target/ppc/kvm: Skip ".." directory in kvmppc_find_cpu_dt

Posted by Daniel Henrique Barboza 3 years, 7 months ago


On 7/12/22 00:46, David Gibson wrote:
> On Mon, Jul 11, 2022 at 04:37:43PM -0300, Murilo Opsfelder Araujo wrote:
>> Some systems have /proc/device-tree/cpus/../clock-frequency. However,
>> this is not the expected path for a CPU device tree directory.
>>
>> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>>   target/ppc/kvm.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 6eed466f80..c8485a5cc0 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -1877,6 +1877,12 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
>>       buf[0] = '\0';
>>       while ((dirp = readdir(dp)) != NULL) {
>>           FILE *f;
>> +
>> +        /* Don't accidentally read from the upper directory */
>> +        if (strcmp(dirp->d_name, "..") == 0) {
> 
> It might not be causing problems now, but it would be technically more
> correct to also skip ".", wouldn't it?

Given that the use of this function is inside kvmppc_read_int_cpu_dt(), which
is used to read a property that belongs to a CPU node, I believe you're right.
It's better to avoid returning "PROC_DEVTREE_CPU" as well.

Murilo, can you please re-send it skipping both ".." and "." ? Better be
on the safe side.


Daniel


> 
>> +            continue;
>> +        }
>> +
>>           snprintf(buf, buf_len, "%s%s/clock-frequency", PROC_DEVTREE_CPU,
>>                    dirp->d_name);
>>           f = fopen(buf, "r");
>

Re: [PATCH] target/ppc/kvm: Skip ".." directory in kvmppc_find_cpu_dt

Posted by Murilo Opsfelder Araújo 3 years, 7 months ago

Hi, Daniel, David.

On 7/12/22 10:03, Daniel Henrique Barboza wrote:
> 
> 
> On 7/12/22 00:46, David Gibson wrote:
>> On Mon, Jul 11, 2022 at 04:37:43PM -0300, Murilo Opsfelder Araujo wrote:
>>> Some systems have /proc/device-tree/cpus/../clock-frequency. However,
>>> this is not the expected path for a CPU device tree directory.
>>>
>>> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>>> ---
>>>   target/ppc/kvm.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>> index 6eed466f80..c8485a5cc0 100644
>>> --- a/target/ppc/kvm.c
>>> +++ b/target/ppc/kvm.c
>>> @@ -1877,6 +1877,12 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
>>>       buf[0] = '\0';
>>>       while ((dirp = readdir(dp)) != NULL) {
>>>           FILE *f;
>>> +
>>> +        /* Don't accidentally read from the upper directory */
>>> +        if (strcmp(dirp->d_name, "..") == 0) {
>>
>> It might not be causing problems now, but it would be technically more
>> correct to also skip ".", wouldn't it?
> 
> Given that the use of this function is inside kvmppc_read_int_cpu_dt(), which
> is used to read a property that belongs to a CPU node, I believe you're right.
> It's better to avoid returning "PROC_DEVTREE_CPU" as well.
> 
> Murilo, can you please re-send it skipping both ".." and "." ? Better be
> on the safe side.
> 
> 
> Daniel

I've sent v2:

     https://lore.kernel.org/qemu-devel/20220712210810.35514-1-muriloo@linux.ibm.com/

Thank you for reviewing.

-- 
Murilo

Re: [PATCH] target/ppc/kvm: Skip ".." directory in kvmppc_find_cpu_dt

Posted by Daniel Henrique Barboza 3 years, 7 months ago


On 7/11/22 16:37, Murilo Opsfelder Araujo wrote:
> Some systems have /proc/device-tree/cpus/../clock-frequency. However,
> this is not the expected path for a CPU device tree directory.
> 
> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   target/ppc/kvm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 6eed466f80..c8485a5cc0 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1877,6 +1877,12 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
>       buf[0] = '\0';
>       while ((dirp = readdir(dp)) != NULL) {
>           FILE *f;
> +
> +        /* Don't accidentally read from the upper directory */
> +        if (strcmp(dirp->d_name, "..") == 0) {
> +            continue;
> +        }
> +
>           snprintf(buf, buf_len, "%s%s/clock-frequency", PROC_DEVTREE_CPU,
>                    dirp->d_name);
>           f = fopen(buf, "r");