A race condition may occur if the user physically removes the
phantom device while calling open().
This is a race condition between phantom_open() function and
the phantom_remove() function, which may lead to Use-After-Free.
Therefore, add a refcount check to phantom_remove() function
to free the "phantom_device" structure after the device is close()d.
---------------CPU 0--------------------CPU 1-----------------
phantom_open() | phantom_remove()
--------------------------------------------------------------
struct phantom_device *dev = |
container_of(inode->i_cdev, |
struct phantom_device, cdev); — (1) |
| struct phantom_device *pht
| = pci_get_drvdata(pdev);
| ...
| kfree(pht); — (2)
if (dev->opened) { — (3) |
Signed-off-by: Yoochan Lee <[email protected]>
---
drivers/misc/phantom.c | 50 ++++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
index ce72e46a2e73..ee75aa1f56ae 100644
--- a/drivers/misc/phantom.c
+++ b/drivers/misc/phantom.c
@@ -55,6 +55,7 @@ struct phantom_device {
/* used in NOT_OH mode */
struct phm_regs oregs;
u32 ctl_reg;
+ struct kref refcnt;
};
static unsigned char phantom_devices[PHANTOM_MAX_MINORS];
@@ -78,6 +79,32 @@ static int phantom_status(struct phantom_device *dev, unsigned long newstat)
return 0;
}
+static void phantom_delete(struct kref *kref)
+{
+ struct phantom_device *pht = container_of(kref, struct phantom_device, refcnt);
+
+ device_destroy(phantom_class, MKDEV(phantom_major, minor));
+
+ cdev_del(&pht->cdev);
+
+ iowrite32(0, pht->caddr + PHN_IRQCTL);
+ ioread32(pht->caddr + PHN_IRQCTL); /* PCI posting */
+ free_irq(pdev->irq, pht);
+
+ pci_iounmap(pdev, pht->oaddr);
+ pci_iounmap(pdev, pht->iaddr);
+ pci_iounmap(pdev, pht->caddr);
+
+ kfree(pht);
+
+ pci_release_regions(pdev);
+
+ phantom_devices[minor] = 0;
+
+ pci_disable_device(pdev);
+
+}
+
/*
* File ops
*/
@@ -232,6 +259,7 @@ static int phantom_open(struct inode *inode, struct file *file)
atomic_set(&dev->counter, 0);
dev->opened++;
+ kref_get(&dev->refcnt);
mutex_unlock(&dev->open_lock);
mutex_unlock(&phantom_mutex);
return 0;
@@ -247,6 +275,7 @@ static int phantom_release(struct inode *inode, struct file *file)
phantom_status(dev, dev->status & ~PHB_RUNNING);
dev->status &= ~PHB_NOT_OH;
+ kref_put(&dev->refcnt, phantom_delete);
mutex_unlock(&dev->open_lock);
return 0;
@@ -384,6 +413,7 @@ static int phantom_probe(struct pci_dev *pdev,
mutex_init(&pht->open_lock);
spin_lock_init(&pht->regs_lock);
+ kref_init(&pht->refcnt);
init_waitqueue_head(&pht->wait);
cdev_init(&pht->cdev, &phantom_file_ops);
pht->cdev.owner = THIS_MODULE;
@@ -436,25 +466,7 @@ static void phantom_remove(struct pci_dev *pdev)
struct phantom_device *pht = pci_get_drvdata(pdev);
unsigned int minor = MINOR(pht->cdev.dev);
- device_destroy(phantom_class, MKDEV(phantom_major, minor));
-
- cdev_del(&pht->cdev);
-
- iowrite32(0, pht->caddr + PHN_IRQCTL);
- ioread32(pht->caddr + PHN_IRQCTL); /* PCI posting */
- free_irq(pdev->irq, pht);
-
- pci_iounmap(pdev, pht->oaddr);
- pci_iounmap(pdev, pht->iaddr);
- pci_iounmap(pdev, pht->caddr);
-
- kfree(pht);
-
- pci_release_regions(pdev);
-
- phantom_devices[minor] = 0;
-
- pci_disable_device(pdev);
+ kref_put(&pht->refcnt, phantom_delete);
}
static int __maybe_unused phantom_suspend(struct device *dev_d)
--
2.39.0
Hi Yoochan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-phantom-Fix-use-after-free-in-phantom_open/20221231-140519
patch link: https://lore.kernel.org/r/20221231060459.2041173-1-yoochan1026%40gmail.com
patch subject: [PATCH] misc: phantom: Fix use-after-free in phantom_open
config: x86_64-allyesconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/350dcfa1ef8b394d79ef66c82a0ec66b813708ef
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yoochan-Lee/misc-phantom-Fix-use-after-free-in-phantom_open/20221231-140519
git checkout 350dcfa1ef8b394d79ef66c82a0ec66b813708ef
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/linux/fs.h:7,
from include/linux/compat.h:17,
from drivers/misc/phantom.c:11:
drivers/misc/phantom.c: In function 'phantom_delete':
drivers/misc/phantom.c:86:60: error: 'minor' undeclared (first use in this function); did you mean 'iminor'?
86 | device_destroy(phantom_class, MKDEV(phantom_major, minor));
| ^~~~~
include/linux/kdev_t.h:12:49: note: in definition of macro 'MKDEV'
12 | #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
| ^~
drivers/misc/phantom.c:86:60: note: each undeclared identifier is reported only once for each function it appears in
86 | device_destroy(phantom_class, MKDEV(phantom_major, minor));
| ^~~~~
include/linux/kdev_t.h:12:49: note: in definition of macro 'MKDEV'
12 | #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
| ^~
drivers/misc/phantom.c:92:18: error: 'pdev' undeclared (first use in this function); did you mean 'cdev'?
92 | free_irq(pdev->irq, pht);
| ^~~~
| cdev
drivers/misc/phantom.c: In function 'phantom_remove':
>> drivers/misc/phantom.c:467:22: warning: unused variable 'minor' [-Wunused-variable]
467 | unsigned int minor = MINOR(pht->cdev.dev);
| ^~~~~
vim +/minor +467 drivers/misc/phantom.c
cef2cf07273d12 Jiri Slaby 2007-05-08 463
486a5c28c2e7d6 Bill Pemberton 2012-11-19 464 static void phantom_remove(struct pci_dev *pdev)
cef2cf07273d12 Jiri Slaby 2007-05-08 465 {
cef2cf07273d12 Jiri Slaby 2007-05-08 466 struct phantom_device *pht = pci_get_drvdata(pdev);
cef2cf07273d12 Jiri Slaby 2007-05-08 @467 unsigned int minor = MINOR(pht->cdev.dev);
cef2cf07273d12 Jiri Slaby 2007-05-08 468
350dcfa1ef8b39 Yoochan Lee 2022-12-31 469 kref_put(&pht->refcnt, phantom_delete);
cef2cf07273d12 Jiri Slaby 2007-05-08 470 }
cef2cf07273d12 Jiri Slaby 2007-05-08 471
--
0-DAY CI Kernel Test Service
https://01.org/lkp
I missed the error in my patches.
Here is the fixed version of the patches.
From c6fb094d3dcf7c4441924cb0a56ba739ed3b2d44 Mon Sep 17 00:00:00 2001
From: Yoochan Lee <[email protected]>
Date: Sat, 31 Dec 2022 18:49:03 +0900
Subject: [PATCH 2/2] fix error in previous patches
---
drivers/misc/phantom.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/phantom.c b/drivers/misc/phantom.c
index ee75aa1f56ae..37c26e728be1 100644
--- a/drivers/misc/phantom.c
+++ b/drivers/misc/phantom.c
@@ -55,6 +55,7 @@ struct phantom_device {
/* used in NOT_OH mode */
struct phm_regs oregs;
u32 ctl_reg;
+ struct pci_dev *pdev;
struct kref refcnt;
};
@@ -82,6 +83,8 @@ static int phantom_status(struct phantom_device
*dev, unsigned long newstat)
static void phantom_delete(struct kref *kref)
{
struct phantom_device *pht = container_of(kref, struct
phantom_device, refcnt);
+ struct pci_dev *pdev = pht->pdev;
+ unsigned int minor = MINOR(pht->cdev.dev);
device_destroy(phantom_class, MKDEV(phantom_major, minor));
@@ -414,6 +417,7 @@ static int phantom_probe(struct pci_dev *pdev,
mutex_init(&pht->open_lock);
spin_lock_init(&pht->regs_lock);
kref_init(&pht->refcnt);
+ pht->pdev = pdev;
init_waitqueue_head(&pht->wait);
cdev_init(&pht->cdev, &phantom_file_ops);
pht->cdev.owner = THIS_MODULE;
@@ -464,7 +468,6 @@ static int phantom_probe(struct pci_dev *pdev,
static void phantom_remove(struct pci_dev *pdev)
{
struct phantom_device *pht = pci_get_drvdata(pdev);
- unsigned int minor = MINOR(pht->cdev.dev);
kref_put(&pht->refcnt, phantom_delete);
}
--
2.39.0
2022년 12월 31일 (토) 오후 6:31, kernel test robot <[email protected]>님이 작성:
>
> Hi Yoochan,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on char-misc/char-misc-testing]
> [also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-phantom-Fix-use-after-free-in-phantom_open/20221231-140519
> patch link: https://lore.kernel.org/r/20221231060459.2041173-1-yoochan1026%40gmail.com
> patch subject: [PATCH] misc: phantom: Fix use-after-free in phantom_open
> config: x86_64-allyesconfig
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> reproduce (this is a W=1 build):
> # https://github.com/intel-lab-lkp/linux/commit/350dcfa1ef8b394d79ef66c82a0ec66b813708ef
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Yoochan-Lee/misc-phantom-Fix-use-after-free-in-phantom_open/20221231-140519
> git checkout 350dcfa1ef8b394d79ef66c82a0ec66b813708ef
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> make W=1 O=build_dir ARCH=x86_64 olddefconfig
> make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/linux/fs.h:7,
> from include/linux/compat.h:17,
> from drivers/misc/phantom.c:11:
> drivers/misc/phantom.c: In function 'phantom_delete':
> drivers/misc/phantom.c:86:60: error: 'minor' undeclared (first use in this function); did you mean 'iminor'?
> 86 | device_destroy(phantom_class, MKDEV(phantom_major, minor));
> | ^~~~~
> include/linux/kdev_t.h:12:49: note: in definition of macro 'MKDEV'
> 12 | #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
> | ^~
> drivers/misc/phantom.c:86:60: note: each undeclared identifier is reported only once for each function it appears in
> 86 | device_destroy(phantom_class, MKDEV(phantom_major, minor));
> | ^~~~~
> include/linux/kdev_t.h:12:49: note: in definition of macro 'MKDEV'
> 12 | #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
> | ^~
> drivers/misc/phantom.c:92:18: error: 'pdev' undeclared (first use in this function); did you mean 'cdev'?
> 92 | free_irq(pdev->irq, pht);
> | ^~~~
> | cdev
> drivers/misc/phantom.c: In function 'phantom_remove':
> >> drivers/misc/phantom.c:467:22: warning: unused variable 'minor' [-Wunused-variable]
> 467 | unsigned int minor = MINOR(pht->cdev.dev);
> | ^~~~~
>
>
> vim +/minor +467 drivers/misc/phantom.c
>
> cef2cf07273d12 Jiri Slaby 2007-05-08 463
> 486a5c28c2e7d6 Bill Pemberton 2012-11-19 464 static void phantom_remove(struct pci_dev *pdev)
> cef2cf07273d12 Jiri Slaby 2007-05-08 465 {
> cef2cf07273d12 Jiri Slaby 2007-05-08 466 struct phantom_device *pht = pci_get_drvdata(pdev);
> cef2cf07273d12 Jiri Slaby 2007-05-08 @467 unsigned int minor = MINOR(pht->cdev.dev);
> cef2cf07273d12 Jiri Slaby 2007-05-08 468
> 350dcfa1ef8b39 Yoochan Lee 2022-12-31 469 kref_put(&pht->refcnt, phantom_delete);
> cef2cf07273d12 Jiri Slaby 2007-05-08 470 }
> cef2cf07273d12 Jiri Slaby 2007-05-08 471
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
Hi Yoochan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus soc/for-next linus/master v6.2-rc1 next-20221226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yoochan-Lee/misc-phantom-Fix-use-after-free-in-phantom_open/20221231-140519
patch link: https://lore.kernel.org/r/20221231060459.2041173-1-yoochan1026%40gmail.com
patch subject: [PATCH] misc: phantom: Fix use-after-free in phantom_open
config: x86_64-randconfig-a005-20221226
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/350dcfa1ef8b394d79ef66c82a0ec66b813708ef
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yoochan-Lee/misc-phantom-Fix-use-after-free-in-phantom_open/20221231-140519
git checkout 350dcfa1ef8b394d79ef66c82a0ec66b813708ef
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/misc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
>> drivers/misc/phantom.c:86:53: error: use of undeclared identifier 'minor'; did you mean 'iminor'?
device_destroy(phantom_class, MKDEV(phantom_major, minor));
^~~~~
iminor
include/linux/kdev_t.h:12:46: note: expanded from macro 'MKDEV'
#define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
^
include/linux/fs.h:890:24: note: 'iminor' declared here
static inline unsigned iminor(const struct inode *inode)
^
>> drivers/misc/phantom.c:86:32: error: invalid operands to binary expression ('int' and 'unsigned int (const struct inode *)')
device_destroy(phantom_class, MKDEV(phantom_major, minor));
^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/kdev_t.h:12:43: note: expanded from macro 'MKDEV'
#define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
~~~~~~~~~~~~~~~~~~~ ^ ~~~~
>> drivers/misc/phantom.c:92:11: error: use of undeclared identifier 'pdev'
free_irq(pdev->irq, pht);
^
drivers/misc/phantom.c:94:14: error: use of undeclared identifier 'pdev'
pci_iounmap(pdev, pht->oaddr);
^
drivers/misc/phantom.c:95:14: error: use of undeclared identifier 'pdev'
pci_iounmap(pdev, pht->iaddr);
^
drivers/misc/phantom.c:96:14: error: use of undeclared identifier 'pdev'
pci_iounmap(pdev, pht->caddr);
^
drivers/misc/phantom.c:100:22: error: use of undeclared identifier 'pdev'
pci_release_regions(pdev);
^
drivers/misc/phantom.c:102:18: error: use of undeclared identifier 'minor'; did you mean 'iminor'?
phantom_devices[minor] = 0;
^~~~~
iminor
include/linux/fs.h:890:24: note: 'iminor' declared here
static inline unsigned iminor(const struct inode *inode)
^
>> drivers/misc/phantom.c:102:17: error: array subscript is not an integer
phantom_devices[minor] = 0;
^~~~~~
drivers/misc/phantom.c:104:21: error: use of undeclared identifier 'pdev'
pci_disable_device(pdev);
^
>> drivers/misc/phantom.c:467:15: warning: unused variable 'minor' [-Wunused-variable]
unsigned int minor = MINOR(pht->cdev.dev);
^
1 warning and 10 errors generated.
vim +86 drivers/misc/phantom.c
81
82 static void phantom_delete(struct kref *kref)
83 {
84 struct phantom_device *pht = container_of(kref, struct phantom_device, refcnt);
85
> 86 device_destroy(phantom_class, MKDEV(phantom_major, minor));
87
88 cdev_del(&pht->cdev);
89
90 iowrite32(0, pht->caddr + PHN_IRQCTL);
91 ioread32(pht->caddr + PHN_IRQCTL); /* PCI posting */
> 92 free_irq(pdev->irq, pht);
93
94 pci_iounmap(pdev, pht->oaddr);
95 pci_iounmap(pdev, pht->iaddr);
96 pci_iounmap(pdev, pht->caddr);
97
98 kfree(pht);
99
100 pci_release_regions(pdev);
101
> 102 phantom_devices[minor] = 0;
103
104 pci_disable_device(pdev);
105
--
0-DAY CI Kernel Test Service
https://01.org/lkp