[PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Zhen Lei]

Ensure that the 'buf' is not empty before strlcpy() uses it.

Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
in kernfs_path_from_node_locked()") first noticed this, but it didn't
fix it completely.

Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 fs/kernfs/dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index f33b3baad07cb36..b2422265c86edf2 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -140,6 +140,9 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
 	size_t depth_from, depth_to, len = 0;
 	int i, j;
 
+	if (!buf)
+		return -EINVAL;
+
 	if (!kn_to)
 		return strlcpy(buf, "(null)", buflen);
 
@@ -149,9 +152,6 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
 	if (kn_from == kn_to)
 		return strlcpy(buf, "/", buflen);
 
-	if (!buf)
-		return -EINVAL;
-
 	common = kernfs_common_ancestor(kn_from, kn_to);
 	if (WARN_ON(!common))
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Tejun Heo]

On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
> Ensure that the 'buf' is not empty before strlcpy() uses it.
> 
> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
> in kernfs_path_from_node_locked()") first noticed this, but it didn't
> fix it completely.
> 
> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

I think the right thing to do is removing that if. It makes no sense to call
that function with NULL buf and the fact that nobody reported crashes on
NULL buf indicates that we in fact never do.

Thanks.

-- 
tejun

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Leizhen (ThunderTown)]

On 2022/11/24 0:55, Tejun Heo wrote:
> On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
>> Ensure that the 'buf' is not empty before strlcpy() uses it.
>>
>> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
>> in kernfs_path_from_node_locked()") first noticed this, but it didn't
>> fix it completely.
>>
>> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> 
> I think the right thing to do is removing that if. It makes no sense to call
> that function with NULL buf and the fact that nobody reported crashes on
> NULL buf indicates that we in fact never do.

OK.

How about I remove "buf[0] = '\0';" too? It seems to be a useless operation.
When 'kn_from' and 'kn_to' have a common ancestor, there must be a path from
'kn_from' to 'kn_to', and strlcpy() always fills in the terminator correctly,
even if the buf is too small to save the first path node.

static void test(void)
{
        char buf[4];
        int i, n, buflen;

        buflen = 1;
        n = strlcpy(buf, "string", buflen);
        for (i = 0; i < buflen; i++)
                printk("%d: %02x\n", i, buf[i]);
        printk("n=%d\n\n", n);

        buflen = sizeof(buf);
        n = strlcpy(buf, "string", buflen);
        for (i = 0; i < buflen; i++)
                printk("%d: %02x\n", i, buf[i]);
        printk("n=%d\n", n);
}

Output:
[   33.691497] 0: 00
[   33.691569] n=6

[   33.691595] 0: 73
[   33.691622] 1: 74
[   33.691630] 2: 72
[   33.691637] 3: 00
[   33.691650] n=6

> 
> Thanks.
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Leizhen (ThunderTown)]

On 2022/11/24 10:24, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/11/24 0:55, Tejun Heo wrote:
>> On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
>>> Ensure that the 'buf' is not empty before strlcpy() uses it.
>>>
>>> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
>>> in kernfs_path_from_node_locked()") first noticed this, but it didn't
>>> fix it completely.
>>>
>>> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> I think the right thing to do is removing that if. It makes no sense to call
>> that function with NULL buf and the fact that nobody reported crashes on
>> NULL buf indicates that we in fact never do.
> 
> OK.
> 
> How about I remove "buf[0] = '\0';" too? It seems to be a useless operation.
> When 'kn_from' and 'kn_to' have a common ancestor, there must be a path from
> 'kn_from' to 'kn_to', and strlcpy() always fills in the terminator correctly,
> even if the buf is too small to save the first path node.

Sorry, I misanalyzed. The length used by "len < buflen ? buflen - len : 0" may
be zero.

> 
> static void test(void)
> {
>         char buf[4];
>         int i, n, buflen;
> 
>         buflen = 1;
>         n = strlcpy(buf, "string", buflen);
>         for (i = 0; i < buflen; i++)
>                 printk("%d: %02x\n", i, buf[i]);
>         printk("n=%d\n\n", n);
> 
>         buflen = sizeof(buf);
>         n = strlcpy(buf, "string", buflen);
>         for (i = 0; i < buflen; i++)
>                 printk("%d: %02x\n", i, buf[i]);
>         printk("n=%d\n", n);
> }
> 
> Output:
> [   33.691497] 0: 00
> [   33.691569] n=6
> 
> [   33.691595] 0: 73
> [   33.691622] 1: 74
> [   33.691630] 2: 72
> [   33.691637] 3: 00
> [   33.691650] n=6
> 
>>
>> Thanks.
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Leizhen (ThunderTown)]

On 2022/11/24 10:28, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/11/24 10:24, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2022/11/24 0:55, Tejun Heo wrote:
>>> On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
>>>> Ensure that the 'buf' is not empty before strlcpy() uses it.
>>>>
>>>> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
>>>> in kernfs_path_from_node_locked()") first noticed this, but it didn't
>>>> fix it completely.
>>>>
>>>> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>
>>> I think the right thing to do is removing that if. It makes no sense to call
>>> that function with NULL buf and the fact that nobody reported crashes on
>>> NULL buf indicates that we in fact never do.
>>
>> OK.
>>
>> How about I remove "buf[0] = '\0';" too? It seems to be a useless operation.
>> When 'kn_from' and 'kn_to' have a common ancestor, there must be a path from
>> 'kn_from' to 'kn_to', and strlcpy() always fills in the terminator correctly,
>> even if the buf is too small to save the first path node.
> 
> Sorry, I misanalyzed. The length used by "len < buflen ? buflen - len : 0" may
> be zero.

Ah, my brain is unstable today. The initial value of len is 0. So "buf[0] = '\0';"
can still be safely removed.

> 
>>
>> static void test(void)
>> {
>>         char buf[4];
>>         int i, n, buflen;
>>
>>         buflen = 1;
>>         n = strlcpy(buf, "string", buflen);
>>         for (i = 0; i < buflen; i++)
>>                 printk("%d: %02x\n", i, buf[i]);
>>         printk("n=%d\n\n", n);
>>
>>         buflen = sizeof(buf);
>>         n = strlcpy(buf, "string", buflen);
>>         for (i = 0; i < buflen; i++)
>>                 printk("%d: %02x\n", i, buf[i]);
>>         printk("n=%d\n", n);
>> }
>>
>> Output:
>> [   33.691497] 0: 00
>> [   33.691569] n=6
>>
>> [   33.691595] 0: 73
>> [   33.691622] 1: 74
>> [   33.691630] 2: 72
>> [   33.691637] 3: 00
>> [   33.691650] n=6
>>
>>>
>>> Thanks.
>>>
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Leizhen (ThunderTown)]

On 2022/11/24 10:52, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/11/24 10:28, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2022/11/24 10:24, Leizhen (ThunderTown) wrote:
>>>
>>>
>>> On 2022/11/24 0:55, Tejun Heo wrote:
>>>> On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
>>>>> Ensure that the 'buf' is not empty before strlcpy() uses it.
>>>>>
>>>>> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
>>>>> in kernfs_path_from_node_locked()") first noticed this, but it didn't
>>>>> fix it completely.
>>>>>
>>>>> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>
>>>> I think the right thing to do is removing that if. It makes no sense to call
>>>> that function with NULL buf and the fact that nobody reported crashes on
>>>> NULL buf indicates that we in fact never do.

kernfs_path_from_node
    -->kernfs_path_from_node_locked

EXPORT_SYMBOL_GPL(kernfs_path_from_node)

I've rethought it. The export APIs need to do null pointer check, right?

>>>
>>> OK.
>>>
>>> How about I remove "buf[0] = '\0';" too? It seems to be a useless operation.
>>> When 'kn_from' and 'kn_to' have a common ancestor, there must be a path from
>>> 'kn_from' to 'kn_to', and strlcpy() always fills in the terminator correctly,
>>> even if the buf is too small to save the first path node.
>>
>> Sorry, I misanalyzed. The length used by "len < buflen ? buflen - len : 0" may
>> be zero.
> 
> Ah, my brain is unstable today. The initial value of len is 0. So "buf[0] = '\0';"
> can still be safely removed.
> 
>>
>>>
>>> static void test(void)
>>> {
>>>         char buf[4];
>>>         int i, n, buflen;
>>>
>>>         buflen = 1;
>>>         n = strlcpy(buf, "string", buflen);
>>>         for (i = 0; i < buflen; i++)
>>>                 printk("%d: %02x\n", i, buf[i]);
>>>         printk("n=%d\n\n", n);
>>>
>>>         buflen = sizeof(buf);
>>>         n = strlcpy(buf, "string", buflen);
>>>         for (i = 0; i < buflen; i++)
>>>                 printk("%d: %02x\n", i, buf[i]);
>>>         printk("n=%d\n", n);
>>> }
>>>
>>> Output:
>>> [   33.691497] 0: 00
>>> [   33.691569] n=6
>>>
>>> [   33.691595] 0: 73
>>> [   33.691622] 1: 74
>>> [   33.691630] 2: 72
>>> [   33.691637] 3: 00
>>> [   33.691650] n=6
>>>
>>>>
>>>> Thanks.
>>>>
>>>
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Greg Kroah-Hartman]

On Sat, Nov 26, 2022 at 05:49:50PM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/11/24 10:52, Leizhen (ThunderTown) wrote:
> > 
> > 
> > On 2022/11/24 10:28, Leizhen (ThunderTown) wrote:
> >>
> >>
> >> On 2022/11/24 10:24, Leizhen (ThunderTown) wrote:
> >>>
> >>>
> >>> On 2022/11/24 0:55, Tejun Heo wrote:
> >>>> On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
> >>>>> Ensure that the 'buf' is not empty before strlcpy() uses it.
> >>>>>
> >>>>> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
> >>>>> in kernfs_path_from_node_locked()") first noticed this, but it didn't
> >>>>> fix it completely.
> >>>>>
> >>>>> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
> >>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>>>
> >>>> I think the right thing to do is removing that if. It makes no sense to call
> >>>> that function with NULL buf and the fact that nobody reported crashes on
> >>>> NULL buf indicates that we in fact never do.
> 
> kernfs_path_from_node
>     -->kernfs_path_from_node_locked
> 
> EXPORT_SYMBOL_GPL(kernfs_path_from_node)
> 
> I've rethought it. The export APIs need to do null pointer check, right?

No, callers should get this right.  Are there any in-tree ones that do
not?

thanks,

greg k-h

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Leizhen (ThunderTown)]

On 2022/11/26 18:25, Greg Kroah-Hartman wrote:
> On Sat, Nov 26, 2022 at 05:49:50PM +0800, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2022/11/24 10:52, Leizhen (ThunderTown) wrote:
>>>
>>>
>>> On 2022/11/24 10:28, Leizhen (ThunderTown) wrote:
>>>>
>>>>
>>>> On 2022/11/24 10:24, Leizhen (ThunderTown) wrote:
>>>>>
>>>>>
>>>>> On 2022/11/24 0:55, Tejun Heo wrote:
>>>>>> On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
>>>>>>> Ensure that the 'buf' is not empty before strlcpy() uses it.
>>>>>>>
>>>>>>> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
>>>>>>> in kernfs_path_from_node_locked()") first noticed this, but it didn't
>>>>>>> fix it completely.
>>>>>>>
>>>>>>> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
>>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>>
>>>>>> I think the right thing to do is removing that if. It makes no sense to call
>>>>>> that function with NULL buf and the fact that nobody reported crashes on
>>>>>> NULL buf indicates that we in fact never do.
>>
>> kernfs_path_from_node
>>     -->kernfs_path_from_node_locked
>>
>> EXPORT_SYMBOL_GPL(kernfs_path_from_node)
>>
>> I've rethought it. The export APIs need to do null pointer check, right?
> 
> No, callers should get this right.  Are there any in-tree ones that do
> not?

Thanks. I got it.

> 
> thanks,
> 
> greg k-h
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kernfs: fix potential null-ptr-deref in kernfs_path_from_node_locked()

[Leizhen (ThunderTown)]

On 2022/11/26 17:49, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/11/24 10:52, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2022/11/24 10:28, Leizhen (ThunderTown) wrote:
>>>
>>>
>>> On 2022/11/24 10:24, Leizhen (ThunderTown) wrote:
>>>>
>>>>
>>>> On 2022/11/24 0:55, Tejun Heo wrote:
>>>>> On Wed, Nov 23, 2022 at 10:04:19AM +0800, Zhen Lei wrote:
>>>>>> Ensure that the 'buf' is not empty before strlcpy() uses it.
>>>>>>
>>>>>> Commit bbe70e4e4211 ("fs: kernfs: Fix possible null-pointer dereferences
>>>>>> in kernfs_path_from_node_locked()") first noticed this, but it didn't
>>>>>> fix it completely.
>>>>>>
>>>>>> Fixes: 9f6df573a404 ("kernfs: Add API to generate relative kernfs path")
>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>
>>>>> I think the right thing to do is removing that if. It makes no sense to call
>>>>> that function with NULL buf and the fact that nobody reported crashes on
>>>>> NULL buf indicates that we in fact never do.
> 
> kernfs_path_from_node
>     -->kernfs_path_from_node_locked
> 
> EXPORT_SYMBOL_GPL(kernfs_path_from_node)
> 
> I've rethought it. The export APIs need to do null pointer check, right?

Sorry，I looked at some of the other functions and they didn't check either.

> 
>>>>
>>>> OK.
>>>>
>>>> How about I remove "buf[0] = '\0';" too? It seems to be a useless operation.
>>>> When 'kn_from' and 'kn_to' have a common ancestor, there must be a path from
>>>> 'kn_from' to 'kn_to', and strlcpy() always fills in the terminator correctly,
>>>> even if the buf is too small to save the first path node.
>>>
>>> Sorry, I misanalyzed. The length used by "len < buflen ? buflen - len : 0" may
>>> be zero.
>>
>> Ah, my brain is unstable today. The initial value of len is 0. So "buf[0] = '\0';"
>> can still be safely removed.
>>
>>>
>>>>
>>>> static void test(void)
>>>> {
>>>>         char buf[4];
>>>>         int i, n, buflen;
>>>>
>>>>         buflen = 1;
>>>>         n = strlcpy(buf, "string", buflen);
>>>>         for (i = 0; i < buflen; i++)
>>>>                 printk("%d: %02x\n", i, buf[i]);
>>>>         printk("n=%d\n\n", n);
>>>>
>>>>         buflen = sizeof(buf);
>>>>         n = strlcpy(buf, "string", buflen);
>>>>         for (i = 0; i < buflen; i++)
>>>>                 printk("%d: %02x\n", i, buf[i]);
>>>>         printk("n=%d\n", n);
>>>> }
>>>>
>>>> Output:
>>>> [   33.691497] 0: 00
>>>> [   33.691569] n=6
>>>>
>>>> [   33.691595] 0: 73
>>>> [   33.691622] 1: 74
>>>> [   33.691630] 2: 72
>>>> [   33.691637] 3: 00
>>>> [   33.691650] n=6
>>>>
>>>>>
>>>>> Thanks.
>>>>>
>>>>
>>>
>>
> 

-- 
Regards,
  Zhen Lei