[PATCH] Input: powermate - fix use-after-free in powermate_config_complete

[PATCH] Input: powermate - fix use-after-free in powermate_config_complete

[Javier Carrasco]

syzbot has found a use-after-free bug [1] in the powermate driver. This
happens when the device is disconnected, which leads to a memory free
from the powermate_device struct.
When an asynchronous control message completes after the kfree and its
callback is invoked, the lock does not exist anymore and hence the bug.

Return immediately if the URB status is -ESHUTDOWN (the actual status
that triggered this bug) or -ENOENT, avoiding any access to potentially
freed memory.

[1] https://syzkaller.appspot.com/bug?extid=0434ac83f907a1dbdd1e

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reported-by: syzbot+0434ac83f907a1dbdd1e@syzkaller.appspotmail.com
---
 drivers/input/misc/powermate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
index c1c733a9cb89..f61333fea35f 100644
--- a/drivers/input/misc/powermate.c
+++ b/drivers/input/misc/powermate.c
@@ -196,8 +196,11 @@ static void powermate_config_complete(struct urb *urb)
 	struct powermate_device *pm = urb->context;
 	unsigned long flags;
 
-	if (urb->status)
+	if (urb->status) {
 		printk(KERN_ERR "powermate: config urb returned %d\n", urb->status);
+		if (status == -ENOENT || status == -ESHUTDOWN)
+			return;
+	}
 
 	spin_lock_irqsave(&pm->lock, flags);
 	powermate_sync_state(pm);

---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230916-topic-powermate_use_after_free-c703c7969c91

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete

[Dmitry Torokhov]

On Mon, Sep 18, 2023 at 06:51:49AM +0200, Javier Carrasco Cruz wrote:
> Hi,
> 
> There's an obvious error in the patch I introduced when cleaningup
> (urb->status should be used instead of just status). I will send a v2.

I think what we need is call to usb_kill_urb(pm->config) in
powermate_disconnect(), right after call to input_unregister_device().

Thanks.

-- 
Dmitry

Re: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete

[kernel test robot]

Hi Javier,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0bb80ecc33a8fb5a682236443c1e740d5c917d1d]

url:    https://github.com/intel-lab-lkp/linux/commits/Javier-Carrasco/Input-powermate-fix-use-after-free-in-powermate_config_complete/20230917-052943
base:   0bb80ecc33a8fb5a682236443c1e740d5c917d1d
patch link:    https://lore.kernel.org/r/20230916-topic-powermate_use_after_free-v1-1-2ffa46652869%40gmail.com
patch subject: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete
config: powerpc-ppc6xx_defconfig (https://download.01.org/0day-ci/archive/20230921/202309210232.d7MpKEIm-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230921/202309210232.d7MpKEIm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309210232.d7MpKEIm-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/input/misc/powermate.c: In function 'powermate_config_complete':
>> drivers/input/misc/powermate.c:201:21: error: 'status' undeclared (first use in this function); did you mean 'kstatfs'?
     201 |                 if (status == -ENOENT || status == -ESHUTDOWN)
         |                     ^~~~~~
         |                     kstatfs
   drivers/input/misc/powermate.c:201:21: note: each undeclared identifier is reported only once for each function it appears in


vim +201 drivers/input/misc/powermate.c

   192	
   193	/* Called when our asynchronous control message completes. We may need to issue another immediately */
   194	static void powermate_config_complete(struct urb *urb)
   195	{
   196		struct powermate_device *pm = urb->context;
   197		unsigned long flags;
   198	
   199		if (urb->status) {
   200			printk(KERN_ERR "powermate: config urb returned %d\n", urb->status);
 > 201			if (status == -ENOENT || status == -ESHUTDOWN)
   202				return;
   203		}
   204	
   205		spin_lock_irqsave(&pm->lock, flags);
   206		powermate_sync_state(pm);
   207		spin_unlock_irqrestore(&pm->lock, flags);
   208	}
   209	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete

[Javier Carrasco]

Hi Dmitry,

On 19.09.23 00:10, Dmitry Torokhov wrote:
> On Mon, Sep 18, 2023 at 06:51:49AM +0200, Javier Carrasco Cruz wrote:
>> Hi,
>>
>> There's an obvious error in the patch I introduced when cleaningup
>> (urb->status should be used instead of just status). I will send a v2.
> 
> I think what we need is call to usb_kill_urb(pm->config) in
> powermate_disconnect(), right after call to input_unregister_device().
> 
> Thanks.
> That is definitely a more meaningful and elegant solution, so I will
check it out and eventually send a v2 with it if everything seems ok. On
the other hand usb_kill_urb() is already used on pm->irq before calling
input_unregister_device(), so I would move the existing usb_kill_urb to
have both calls right after the unregister_device call for code
consistency, if that is alright.

Thanks and best regards.