[PATCH v2] eulerfs: fix wrong dereferencing in eufs_lookup
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA -------------------------------- smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err. Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- v2 -> v1: use correct string format fs/eulerfs/namei.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..dd3bbc0b453c 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -115,8 +115,8 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry, inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); + eufs_err(dir->i_sb, "deleted inode referenced: 0x%llx", + le64_to_cpu(de->inode)); return ERR_PTR(-EIO); } not_found: -- 2.34.1
Hi, 在 2023/04/10 20:55, Kang Chen 写道:
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA
--------------------------------
smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err.
Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- v2 -> v1: use correct string format
fs/eulerfs/namei.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..dd3bbc0b453c 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -115,8 +115,8 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry,
inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); + eufs_err(dir->i_sb, "deleted inode referenced: 0x%llx", + le64_to_cpu(de->inode));
Thanks for the patch, can you also fix that follow up dereference of inode will panic if eufs_iget() return ERR_PTR(-ENOMEM). By the way, I think it's impossible to return -ESTALE, perhaps this can be removed. Thansk, Kuai
return ERR_PTR(-EIO); } not_found:
-----Original Messages----- From: "Yu Kuai" <yukuai3@huawei.com> Sent Time: 2023-04-10 21:45:50 (Monday) To: "Kang Chen" <void0red@hust.edu.cn>, kernel@openeuler.org Cc: Subject: Re: [PATCH v2] eulerfs: fix wrong dereferencing in eufs_lookup
Hi,
在 2023/04/10 20:55, Kang Chen 写道:
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA
--------------------------------
smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err.
Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- v2 -> v1: use correct string format
fs/eulerfs/namei.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..dd3bbc0b453c 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -115,8 +115,8 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry,
inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); + eufs_err(dir->i_sb, "deleted inode referenced: 0x%llx", + le64_to_cpu(de->inode));
Thanks for the patch, can you also fix that follow up dereference of inode will panic if eufs_iget() return ERR_PTR(-ENOMEM).
By the way, I think it's impossible to return -ESTALE, perhaps this can be removed. To be honest, I'm not familiar with eulerfs. But eufs_read_pinode called by eufs_iget can return -ESTALE. I'm not sure how to gracefully handle errors here. Or just throw a log.
Thansk, Kuai
return ERR_PTR(-EIO); } not_found:
-- best regards, Kang Chen
Hi, 在 2023/04/10 22:52, 陈康 写道:
-----Original Messages----- From: "Yu Kuai" <yukuai3@huawei.com> Sent Time: 2023-04-10 21:45:50 (Monday) To: "Kang Chen" <void0red@hust.edu.cn>, kernel@openeuler.org Cc: Subject: Re: [PATCH v2] eulerfs: fix wrong dereferencing in eufs_lookup
Hi,
在 2023/04/10 20:55, Kang Chen 写道:
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA
--------------------------------
smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err.
Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- v2 -> v1: use correct string format
fs/eulerfs/namei.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..dd3bbc0b453c 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -115,8 +115,8 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry,
inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); + eufs_err(dir->i_sb, "deleted inode referenced: 0x%llx", + le64_to_cpu(de->inode));
Thanks for the patch, can you also fix that follow up dereference of inode will panic if eufs_iget() return ERR_PTR(-ENOMEM).
By the way, I think it's impossible to return -ESTALE, perhaps this can be removed. To be honest, I'm not familiar with eulerfs. But eufs_read_pinode called by eufs_iget can return -ESTALE. I'm not sure how to gracefully handle errors here. Or just throw a log.
It's right the code is written this way, I mean that logically the condition in eufs_read_pinode should never pass, hence it's deadcode... Any way, your change is fine for me. Can you send a new version to checking inode is not error before dereference it? Thanks, Kuai
Thansk, Kuai
return ERR_PTR(-EIO); } not_found:
-- best regards, Kang Chen
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA -------------------------------- smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err. Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- If we need to pass all errors? v3 -> v2: use IS_ERR to handle all kind err v2 -> v1: use correct string format fs/eulerfs/namei.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..9ef2da748a54 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -114,11 +114,12 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry, goto not_found; inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); - if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); - return ERR_PTR(-EIO); + if (IS_ERR(inode)) { + eufs_err(dir->i_sb, "eufs_iget failed ino 0x%llx err %d\n", + le64_to_cpu(de->inode), inode); + return inode; } + not_found: if (inode) -- 2.34.1
Hi, 在 2023/04/11 11:19, Kang Chen 写道:
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA
--------------------------------
smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err.
Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- If we need to pass all errors?
v3 -> v2: use IS_ERR to handle all kind err v2 -> v1: use correct string format
fs/eulerfs/namei.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..9ef2da748a54 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -114,11 +114,12 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry, goto not_found;
inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); - if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); - return ERR_PTR(-EIO); + if (IS_ERR(inode)) { + eufs_err(dir->i_sb, "eufs_iget failed ino 0x%llx err %d\n", + le64_to_cpu(de->inode), inode);
Please keep the return value as before, only return -EIO or -ENOMEM to user. And PTR_ERR(inode) should be used here. By the way, de->inode is the address of eufs_inode, it's better to show user the actual i_ino. Thanks, Kuai
+ return inode; } + not_found:
if (inode)
-----原始邮件----- 发件人: "Yu Kuai" <yukuai3@huawei.com> 发送时间: 2023-04-12 15:48:58 (星期三) 收件人: "Kang Chen" <void0red@hust.edu.cn> 抄送: kernel@openeuler.org 主题: Re: [PATCH v3] eulerfs: fix wrong dereferencing in eufs_lookup
Hi,
在 2023/04/11 11:19, Kang Chen 写道:
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA
--------------------------------
smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err.
Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- If we need to pass all errors?
v3 -> v2: use IS_ERR to handle all kind err v2 -> v1: use correct string format
fs/eulerfs/namei.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..9ef2da748a54 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -114,11 +114,12 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry, goto not_found;
inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); - if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); - return ERR_PTR(-EIO); + if (IS_ERR(inode)) { + eufs_err(dir->i_sb, "eufs_iget failed ino 0x%llx err %d\n", + le64_to_cpu(de->inode), inode);
Please keep the return value as before, only return -EIO or -ENOMEM to user. And PTR_ERR(inode) should be used here.
By the way, de->inode is the address of eufs_inode, it's better to show user the actual i_ino.
What is the actual i_ino mean? I find "le64_to_cpu(de->inode) == inode->i_ino" in eufs_valid_inode_in_de, and replace the inode->i_ino with le64_to_cpu(de->inode). Or use eufs_pi2ino(sb, s2p(dir->i_sb, de->inode)) to get i_ino?
Thanks, Kuai
+ return inode; } + not_found:
if (inode)
Hi, 在 2023/04/12 17:04, 陈康 写道:
-----原始邮件----- 发件人: "Yu Kuai" <yukuai3@huawei.com> 发送时间: 2023-04-12 15:48:58 (星期三) 收件人: "Kang Chen" <void0red@hust.edu.cn> 抄送: kernel@openeuler.org 主题: Re: [PATCH v3] eulerfs: fix wrong dereferencing in eufs_lookup
Hi,
在 2023/04/11 11:19, Kang Chen 写道:
uniontech inclusion category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I6TN56 CVE: NA
--------------------------------
smatch report: fs/eulerfs/namei.c:118 eufs_lookup() error: 'inode' dereferencing possible ERR_PTR() fix it by using the ino above in eufs_err.
Signed-off-by: Kang Chen <void0red@hust.edu.cn> --- If we need to pass all errors?
v3 -> v2: use IS_ERR to handle all kind err v2 -> v1: use correct string format
fs/eulerfs/namei.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/eulerfs/namei.c b/fs/eulerfs/namei.c index e4c6c36575f2..9ef2da748a54 100644 --- a/fs/eulerfs/namei.c +++ b/fs/eulerfs/namei.c @@ -114,11 +114,12 @@ static struct dentry *eufs_lookup(struct inode *dir, struct dentry *dentry, goto not_found;
inode = eufs_iget(dir->i_sb, s2p(dir->i_sb, de->inode)); - if (inode == ERR_PTR(-ESTALE)) { - eufs_err(dir->i_sb, "deleted inode referenced: 0x%lx", - inode->i_ino); - return ERR_PTR(-EIO); + if (IS_ERR(inode)) { + eufs_err(dir->i_sb, "eufs_iget failed ino 0x%llx err %d\n", + le64_to_cpu(de->inode), inode);
Please keep the return value as before, only return -EIO or -ENOMEM to user. And PTR_ERR(inode) should be used here.
By the way, de->inode is the address of eufs_inode, it's better to show user the actual i_ino.
What is the actual i_ino mean? I find "le64_to_cpu(de->inode) == inode->i_ino" in eufs_valid_inode_in_de, and replace the inode->i_ino with le64_to_cpu(de->inode). Or use eufs_pi2ino(sb, s2p(dir->i_sb, de->inode)) to get i_ino?
Sorry I was looking at dep_new_insert() and I confuse them... It's right de->inode is set to i_ino in nv_dict_add(). Thanks, Kuai
Thanks, Kuai
+ return inode; } + not_found:
if (inode)
participants (3)
-
Kang Chen -
Yu Kuai -
陈康