hfsplus: fix NULL dereference in hfsplus_lookup()
Subsystems (2)
PATCH 1/1 hfsplus: fix NULL dereference in hfsplus_lookup()
Check that the hidden directory is not NULL before using it, instead of after.
--- fs/hfsplus/dir.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fs/hfsplus/dir.c +2 -2
@@ -78,13 +78,13 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry,cpu_to_be32 (HFSP_HARDLINK_TYPE ) && entry.file.user_info.fdCreator ==cpu_to_be32 (HFSP_HFSPLUS_CREATOR ) && +HFSPLUS_SB (sb)->hidden_dir && (entry.file.create_date ==HFSPLUS_I (HFSPLUS_SB (sb)->hidden_dir)-> create_date || entry.file.create_date ==HFSPLUS_I (d_inode (sb->s_root))-> - create_date) && - HFSPLUS_SB(sb)->hidden_dir) { + create_date)) { struct qstr str;char name[32 ];
Signature · 1 line
2.11.0
Viacheslav Dubeyko
maintainerQuoted 6 lines · Ernesto A. Fernández
Check that the hidden directory is not NULL before using it, instead of after.
Reported-by Wen Xu <[email protected]>Signed-off-by Ernesto A. Fernández <[email protected]>---
It's really hard to understand this simple patch. I believe it makes sense to rework the patch slightly with the goal to make it more clear. Also, it will be great to add a short comment in the code to explain what's wrong.
I think it makes sense to split this long check condition on something more clear, simple and elegant.
Thanks,
Vyacheslav Dubeyko.
Quoted 23 lines
fs/hfsplus/dir.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)fs/hfsplus/dir.c +2
-2@@ -78,13 +78,13 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry,cpu_to_be32 (HFSP_HARDLINK_TYPE ) && entry.file.user_info.fdCreator ==cpu_to_be32 (HFSP_HFSPLUS_CREATOR ) && +HFSPLUS_SB (sb)->hidden_dir && (entry.file.create_date ==HFSPLUS_I (HFSPLUS_SB (sb)->hidden_dir)-> create_date || entry.file.create_date ==HFSPLUS_I (d_inode (sb->s_root))-> - create_date) && - HFSPLUS_SB(sb)->hidden_dir) { + create_date)) { struct qstr str;char name[32 ];
Quoted 8 lines · On Thu, 12 Jul 2018 15:55:33 -0700 Viacheslav Dubeyko <[email protected]> wrote:
Quoted 6 lines · Ernesto A. Fern�ndez
Check that the hidden directory is not NULL before using it, instead of after.
Reported-by Wen Xu <[email protected]>Signed-off-by Ernesto A. Fern�ndez <[email protected]>---
Quoted 7 lines
It's really hard to understand this simple patch. I believe it makes sense to rework the patch slightly with the goal to make it more clear. Also, it will be great to add a short comment in the code to explain what's wrong.
I think it makes sense to split this long check condition on something more clear, simple and elegant.
No response, causing this patch to be stuck in limbo land?
Quoted 6 lines · Andrew Morton
Quoted 4 lines · On Thu, 12 Jul 2018 15:55:33 -0700 Viacheslav Dubeyko <[email protected]> wrote:
Quoted 3 lines · Ernesto A. Fernández
Check that the hidden directory is not NULL before using it, instead of after.
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Reported-by Wen Xu <[email protected]>Signed-off-by Ernesto A. Fernández <[email protected]>---
It's really hard to understand this simple patch. I believe it makes sense to rework the patch slightly with the goal to make it more clear. Also, it will be great to add a short comment in the code to explain what's wrong.
I don't think it's reasonable to expect a comment explaining why we can't dereference NULL.
Quoted 2 lines
Quoted 2 lines
I think it makes sense to split this long check condition on something more clear, simple and elegant.
The long check condition may not be ideal, but there's a lot of code in the module that could use style improvements. I don't think that should be a priority right now, with plenty of serious bugs left to fix.
Quoted 2 lines
No response, causing this patch to be stuck in limbo land?
I believe I sent a second version of this patch.
Ernest
Viacheslav Dubeyko
maintainerQuoted 11 lines · Ernesto A. Fernández
Quoted 10 lines · Andrew Morton
Quoted 8 lines · On Thu, 12 Jul 2018 15:55:33 -0700 Viacheslav Dubeyko <[email protected]> wrote:
Quoted 6 lines · Ernesto A. Fernández
Check that the hidden directory is not NULL before using it, instead of after.
Reported-by Wen Xu <[email protected]>Signed-off-by Ernesto A. Fernández <[email protected]>---
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
It's really hard to understand this simple patch. I believe it makes sense to rework the patch slightly with the goal to make it more clear. Also, it will be great to add a short comment in the code to explain what's wrong.
I don't think it's reasonable to expect a comment explaining why we can't dereference NULL.
The good comment is always really important part of the patch.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
I think it makes sense to split this long check condition on something more clear, simple and elegant.
The long check condition may not be ideal, but there's a lot of code in the module that could use style improvements. I don't think that should be a priority right now, with plenty of serious bugs left to fix.
Bad style of code is one of the reason of bugs. If you don't try to improve the code then you can simply create an another serious bug and nobody will be able to understand your fix. The bad style of code in the module is not the excuse at all. It's the way of open-source community to achieve the good style of code by means of the discussion. Moreover, the goal of bug fix is the improvement of code style too but not only to resolve the issue. Another guys need to understand your way of the fix too.
Thanks,
Vyacheslav Dubeyko.
Quoted 7 lines
Quoted 2 lines
No response, causing this patch to be stuck in limbo land?
I believe I sent a second version of this patch.
Ernest
Quoted 17 lines · Viacheslav Dubeyko
Quoted 16 lines · Ernesto A. Fernández
Quoted 14 lines · Andrew Morton
Quoted 12 lines · On Thu, 12 Jul 2018 15:55:33 -0700 Viacheslav Dubeyko <[email protected]> wrote:
Quoted 6 lines · Ernesto A. Fernández
Check that the hidden directory is not NULL before using it, instead of after.
Reported-by Wen Xu <[email protected]>Signed-off-by Ernesto A. Fernández <[email protected]>---
It's really hard to understand this simple patch. I believe it makes sense to rework the patch slightly with the goal to make it more clear. Also, it will be great to add a short comment in the code to explain what's wrong.
Quoted 5 lines
Quoted 3 lines
I don't think it's reasonable to expect a comment explaining why we can't dereference NULL.
The good comment is always really important part of the patch.
That's your idea of a good comment?
Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
I think it makes sense to split this long check condition on something more clear, simple and elegant.
The long check condition may not be ideal, but there's a lot of code in the module that could use style improvements. I don't think that should be a priority right now, with plenty of serious bugs left to fix.
Bad style of code is one of the reason of bugs. If you don't try to improve the code then you can simply create an another serious bug and nobody will be able to understand your fix.
All this does is reorder a check. Where could I possibly introduce a bug?
Large unnecessary rewrites with little testing in unmaintained code are far more likely to cause trouble. I will be more comfortable with such things once I get the module to pass xfstests, but there's still plenty of bugs in the way.
Quoted 6 lines
The bad style of code in the
module is not the excuse at all. It's the way of open-source community to achieve the good style of code by means of the discussion. Moreover, the goal of bug fix is the improvement of code style too but not only to resolve the issue. Another guys need to understand your way of the fix too.
This patch couldn't be any simpler. Anybody who finds it confusing would be completely overwhelmed by a big rewrite.
Quoted 12 lines
Thanks,
Vyacheslav Dubeyko.Quoted 7 lines
Quoted 2 lines
No response, causing this patch to be stuck in limbo land?
I believe I sent a second version of this patch.
Ernest
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Thu, 12 Jul 2018 18:53:47 -0300 Ernesto A. Fernández…
Check that the hidden directory is not NULL before using it, instead of after.
Under what circumstances does the crash happen? Mount of a corrupted fs?
No, when looking up corrupted catalog data (dentry) on a filesystem with no metadata directory (this could only ever happen on a read-only mount). Wen Xu sent the replication steps in detail to the fsdevel list, here is a link:
https://bugzilla.kernel.org/show_bug.cgi?id=200297
I did not explain this because I think the diff makes it clear that there is a bug at least.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 16 lines · Andrew Morton
Quoted 14 lines · On Thu, 12 Jul 2018 20:07:51 -0300 Ernesto A. Fernández…
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Thu, 12 Jul 2018 18:53:47 -0300 Ernesto A. Fernández…
Check that the hidden directory is not NULL before using it, instead of after.
Under what circumstances does the crash happen? Mount of a corrupted fs?
No, when looking up corrupted catalog data (dentry) on a filesystem with no metadata directory (this could only ever happen on a read-only mount). Wen Xu sent the replication steps in detail to the fsdevel list, here is a link:
Quoted 9 lines
Quoted 4 lines
https://bugzilla.kernel.org/show_bug.cgi?id=200297
I did not explain this because I think the diff makes it clear that there is a bug at least.
It is important that we know what circumstances lead to the bug being triggered, so that people can decide whether to backport the fix into their various kernels.
OK, I'll resend with more details.