bug report: potential ERR_PTR dereference in iwm_debugfs_init()

DISCUSSION Dan Carpenter 3 participants 5 messages Apr 22, 2010 – Apr 26, 2010

lore

Share & tools

lore

Dan Carpenter

author

2010-04-22 09:59 UTC #0 lore raw

Hi Zhu Yi,

This is a Smatch bug that has me a little puzzled.

drivers/net/wireless/iwmc3200wifi/debugfs.c +447 iwm_debugfs_init(26)

	warn: 'iwm->dbg.devdir' dereferencing possible ERR_PTR()

   440          iwm->dbg.devdir = debugfs_create_dir(devdir, iwm->dbg.rootdir);
   441          result = PTR_ERR(iwm->dbg.devdir);
   442          if (IS_ERR(iwm->dbg.devdir) && (result != -ENODEV)) {

443 IWM_ERR(iwm, "Couldn't create devdir: %d\n", result);
444 goto error;
445 }
446
447 iwm->dbg.dbgdir = debugfs_create_dir("debug", iwm->dbg.devdir);

It looks like "iwm->dbg.devdir" could be ERR_PTR(-ENODEV) on line 447 and that would cause a problem inside debugfs_create_dir(). But at the same time -ENODEV was deliberately singled out as OK from other possible errors that debugfs_create_dir() can return.

I'm confused.

regards,
dan carpenter

Zhu Yi

2010-04-23 02:49 UTC #1 lore raw

Quoted 16 lines · Dan Carpenter

Hi Zhu Yi,

This is a Smatch bug that has me a little puzzled.

drivers/net/wireless/iwmc3200wifi/debugfs.c +447 iwm_debugfs_init(26)

	warn: 'iwm->dbg.devdir' dereferencing possible ERR_PTR()

   440          iwm->dbg.devdir = debugfs_create_dir(devdir, iwm->dbg.rootdir);
   441          result = PTR_ERR(iwm->dbg.devdir);
   442          if (IS_ERR(iwm->dbg.devdir) && (result != -ENODEV)) {
   443                  IWM_ERR(iwm, "Couldn't create devdir: %d\n", result);
   444                  goto error;
   445          }
   446
   447          iwm->dbg.dbgdir = debugfs_create_dir("debug", iwm->dbg.devdir);

Quoted 4 lines

It looks like "iwm->dbg.devdir" could be ERR_PTR(-ENODEV) on line 447 and that would cause a problem inside debugfs_create_dir(). But at the same time -ENODEV was deliberately singled out as OK from other possible errors that debugfs_create_dir() can return.

We take -ENODEV for debugfs_create_dir if CONFIG_DEBUG_FS is not enabled. We returns 0 deliberately in this case for rootdir create. I agree we don't need to check it for the subdirs like we did now. But I found lots of code don't even check (or don't use IS_ERR to check) the return value of debugfs_create_dir. Maybe that's more problematic?

Thanks,
-yi

Dan Carpenter

author

2010-04-23 11:44 UTC #2 lore raw

Quoted 22 lines · Zhu Yi

Quoted 16 lines · Dan Carpenter
Hi Zhu Yi,
This is a Smatch bug that has me a little puzzled.
drivers/net/wireless/iwmc3200wifi/debugfs.c +447 iwm_debugfs_init(26)
	warn: 'iwm->dbg.devdir' dereferencing possible ERR_PTR()
   440          iwm->dbg.devdir = debugfs_create_dir(devdir, iwm->dbg.rootdir);
   441          result = PTR_ERR(iwm->dbg.devdir);
   442          if (IS_ERR(iwm->dbg.devdir) && (result != -ENODEV)) {
   443                  IWM_ERR(iwm, "Couldn't create devdir: %d\n", result);
   444                  goto error;
   445          }
   446
   447          iwm->dbg.dbgdir = debugfs_create_dir("debug", iwm->dbg.devdir);
Quoted 4 lines
It looks like "iwm->dbg.devdir" could be ERR_PTR(-ENODEV) on line 447 and that would cause a problem inside debugfs_create_dir(). But at the same time -ENODEV was deliberately singled out as OK from other possible errors that debugfs_create_dir() can return.

Quoted 5 lines

We take -ENODEV for debugfs_create_dir if CONFIG_DEBUG_FS is not enabled. We returns 0 deliberately in this case for rootdir create. I agree we don't need to check it for the subdirs like we did now. But I found lots of code don't even check (or don't use IS_ERR to check) the return value of debugfs_create_dir. Maybe that's more problematic?

Ah. Thanks for the explanation.

The bit that was problematic in this code for me is that passing ERR_PTR(-ENODEV) to debugfs_create_dir() on line 447 will cause an oops. But, as you point out, the check on line 442 is never true because we already established that debugfs is enabled.

Couldn't we just check "if (debugfs_initialized()) { " and remove all the ERR_PTR checking?

If you would like I can send a patch to do this.

regards,
dan carpenter

Johannes Berg

2010-04-23 12:03 UTC #3 lore raw

Quoted 25 lines · Dan Carpenter

Quoted 19 lines
Quoted 14 lines
This is a Smatch bug that has me a little puzzled.
drivers/net/wireless/iwmc3200wifi/debugfs.c +447 iwm_debugfs_init(26)
	warn: 'iwm->dbg.devdir' dereferencing possible ERR_PTR()
   440          iwm->dbg.devdir = debugfs_create_dir(devdir, iwm->dbg.rootdir);
   441          result = PTR_ERR(iwm->dbg.devdir);
   442          if (IS_ERR(iwm->dbg.devdir) && (result != -ENODEV)) {
   443                  IWM_ERR(iwm, "Couldn't create devdir: %d\n", result);
   444                  goto error;
   445          }
   446
   447          iwm->dbg.dbgdir = debugfs_create_dir("debug", iwm->dbg.devdir);
Quoted 4 lines
It looks like "iwm->dbg.devdir" could be ERR_PTR(-ENODEV) on line 447 and that would cause a problem inside debugfs_create_dir(). But at the same time -ENODEV was deliberately singled out as OK from other possible errors that debugfs_create_dir() can return.
Quoted 5 lines
We take -ENODEV for debugfs_create_dir if CONFIG_DEBUG_FS is not enabled. We returns 0 deliberately in this case for rootdir create. I agree we don't need to check it for the subdirs like we did now. But I found lots of code don't even check (or don't use IS_ERR to check) the return value of debugfs_create_dir. Maybe that's more problematic?

Quoted 6 lines

Ah. Thanks for the explanation.
The bit that was problematic in this code for me is that passing ERR_PTR(-ENODEV) to debugfs_create_dir() on line 447 will cause an oops. But, as you point out, the check on line 442 is never true because we already established that debugfs is enabled.

I don't think so. See, that function will only return -ENODEV when debugfs is not compiled into the kernel, and in that case the argument to debugfs_create_dir is never used anyway.

johannes

Zhu Yi

2010-04-26 03:24 UTC #4 lore raw

Quoted 10 lines · Johannes Berg

Quoted 2 lines
The bit that was problematic in this code for me is that passing ERR_PTR(-ENODEV) to debugfs_create_dir() on line 447 will cause an
oops.
Quoted 1 line
But, as you point out, the check on line 442 is never true because
we
Quoted 1 line
already established that debugfs is enabled.
I don't think so. See, that function will only return -ENODEV when debugfs is not compiled into the kernel, and in that case the argument to debugfs_create_dir is never used anyway.

Yes, there won't be any kernel oops anyway. But the iwm debugfs code is a little misleading. Dan, please feel free to send a patch to clean up this. On the error path, either NULL or ERR_PTR(-ENODEV) (no other errno) is returned. Since all the debugfs functions are defined as no-op if CONFIG_DEBUG_FS is not selected, maybe we only check against NULL is enough.

Thanks,
-yi