All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: mcp-2221: prevent UAF in delayed work
@ 2023-02-15 16:48 ` Benjamin Tissoires
  0 siblings, 0 replies; 3+ messages in thread
From: Benjamin Tissoires via B4 Submission Endpoint @ 2023-02-15 16:48 UTC (permalink / raw)
  To: Rishi Gupta, Jiri Kosina, Pietro Borrello
  Cc: linux-input, linux-kernel, Benjamin Tissoires

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>

If the device is plugged/unplugged without giving time for mcp_init_work()
to complete, we might kick in the devm free code path and thus have
unavailable struct mcp_2221 while in delayed work.

Add a boolean and a spinlock to prevent scheduling the deleyed work if
we are in the operation of removing the device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
Similar to Pietro's series, we can see the pattern in hid-mcp2221,
except that this time the ledclass is not involved.

Link: https://lore.kernel.org/linux-input/20230125-hid-unregister-leds-v4-5-7860c5763c38@diag.uniroma1.it/
---
 drivers/hid/hid-mcp2221.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e61dd039354b..de8b988f4a48 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -95,6 +95,8 @@ struct mcp2221 {
 	struct mutex lock;
 	struct completion wait_in_report;
 	struct delayed_work init_work;
+	spinlock_t init_work_lock;
+	bool removing;
 	u8 *rxbuf;
 	u8 txbuf[64];
 	int rxbuf_idx;
@@ -922,6 +924,14 @@ static void mcp2221_hid_unregister(void *ptr)
 /* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
 static void mcp2221_remove(struct hid_device *hdev)
 {
+	struct mcp2221 *mcp = hid_get_drvdata(hdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&mcp->init_work_lock, flags);
+	mcp->removing = true;
+	spin_unlock_irqrestore(&mcp->init_work_lock, flags);
+
+	cancel_delayed_work_sync(&mcp->init_work);
 }
 
 #if IS_REACHABLE(CONFIG_IIO)
@@ -1040,6 +1050,7 @@ static void mcp_init_work(struct work_struct *work)
 	struct mcp2221_iio *data;
 	static int retries = 5;
 	int ret, num_channels;
+	unsigned long flags;
 
 	hid_hw_power(mcp->hdev, PM_HINT_FULLON);
 	mutex_lock(&mcp->lock);
@@ -1090,7 +1101,10 @@ static void mcp_init_work(struct work_struct *work)
 		return;
 
 	/* Device is not ready to read SRAM or FLASH data, try again */
-	schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
+	spin_lock_irqsave(&mcp->init_work_lock, flags);
+	if (!mcp->removing)
+		schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
+	spin_unlock_irqrestore(&mcp->init_work_lock, flags);
 }
 #endif
 
@@ -1131,6 +1145,7 @@ static int mcp2221_probe(struct hid_device *hdev,
 	}
 
 	mutex_init(&mcp->lock);
+	spin_lock_init(&mcp->init_work_lock);
 	init_completion(&mcp->wait_in_report);
 	hid_set_drvdata(hdev, mcp);
 	mcp->hdev = hdev;

---
base-commit: d883fd110dc17308a1506c5bf17e00ce9fe7b2a2
change-id: 20230215-wip-mcp2221-979d4115efb5

Best regards,
-- 
Benjamin Tissoires <benjamin.tissoires@redhat.com>


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* [PATCH] HID: mcp-2221: prevent UAF in delayed work
@ 2023-02-15 16:48 ` Benjamin Tissoires
  0 siblings, 0 replies; 3+ messages in thread
From: Benjamin Tissoires @ 2023-02-15 16:48 UTC (permalink / raw)
  To: Rishi Gupta, Jiri Kosina, Pietro Borrello
  Cc: linux-input, linux-kernel, Benjamin Tissoires

If the device is plugged/unplugged without giving time for mcp_init_work()
to complete, we might kick in the devm free code path and thus have
unavailable struct mcp_2221 while in delayed work.

Add a boolean and a spinlock to prevent scheduling the deleyed work if
we are in the operation of removing the device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
Similar to Pietro's series, we can see the pattern in hid-mcp2221,
except that this time the ledclass is not involved.

Link: https://lore.kernel.org/linux-input/20230125-hid-unregister-leds-v4-5-7860c5763c38@diag.uniroma1.it/
---
 drivers/hid/hid-mcp2221.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e61dd039354b..de8b988f4a48 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -95,6 +95,8 @@ struct mcp2221 {
 	struct mutex lock;
 	struct completion wait_in_report;
 	struct delayed_work init_work;
+	spinlock_t init_work_lock;
+	bool removing;
 	u8 *rxbuf;
 	u8 txbuf[64];
 	int rxbuf_idx;
@@ -922,6 +924,14 @@ static void mcp2221_hid_unregister(void *ptr)
 /* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
 static void mcp2221_remove(struct hid_device *hdev)
 {
+	struct mcp2221 *mcp = hid_get_drvdata(hdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&mcp->init_work_lock, flags);
+	mcp->removing = true;
+	spin_unlock_irqrestore(&mcp->init_work_lock, flags);
+
+	cancel_delayed_work_sync(&mcp->init_work);
 }
 
 #if IS_REACHABLE(CONFIG_IIO)
@@ -1040,6 +1050,7 @@ static void mcp_init_work(struct work_struct *work)
 	struct mcp2221_iio *data;
 	static int retries = 5;
 	int ret, num_channels;
+	unsigned long flags;
 
 	hid_hw_power(mcp->hdev, PM_HINT_FULLON);
 	mutex_lock(&mcp->lock);
@@ -1090,7 +1101,10 @@ static void mcp_init_work(struct work_struct *work)
 		return;
 
 	/* Device is not ready to read SRAM or FLASH data, try again */
-	schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
+	spin_lock_irqsave(&mcp->init_work_lock, flags);
+	if (!mcp->removing)
+		schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
+	spin_unlock_irqrestore(&mcp->init_work_lock, flags);
 }
 #endif
 
@@ -1131,6 +1145,7 @@ static int mcp2221_probe(struct hid_device *hdev,
 	}
 
 	mutex_init(&mcp->lock);
+	spin_lock_init(&mcp->init_work_lock);
 	init_completion(&mcp->wait_in_report);
 	hid_set_drvdata(hdev, mcp);
 	mcp->hdev = hdev;

---
base-commit: d883fd110dc17308a1506c5bf17e00ce9fe7b2a2
change-id: 20230215-wip-mcp2221-979d4115efb5

Best regards,
-- 
Benjamin Tissoires <benjamin.tissoires@redhat.com>


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] HID: mcp-2221: prevent UAF in delayed work
  2023-02-15 16:48 ` Benjamin Tissoires
  (?)
@ 2023-02-15 17:15 ` Benjamin Tissoires
  -1 siblings, 0 replies; 3+ messages in thread
From: Benjamin Tissoires @ 2023-02-15 17:15 UTC (permalink / raw)
  To: Rishi Gupta, Jiri Kosina, Pietro Borrello, linux-input, linux-kernel

On Feb 15 2023, Benjamin Tissoires via B4 Submission Endpoint wrote:
> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> If the device is plugged/unplugged without giving time for mcp_init_work()
> to complete, we might kick in the devm free code path and thus have
> unavailable struct mcp_2221 while in delayed work.
> 
> Add a boolean and a spinlock to prevent scheduling the deleyed work if
> we are in the operation of removing the device.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> Similar to Pietro's series, we can see the pattern in hid-mcp2221,
> except that this time the ledclass is not involved.
> 
> Link: https://lore.kernel.org/linux-input/20230125-hid-unregister-leds-v4-5-7860c5763c38@diag.uniroma1.it/
> ---
>  drivers/hid/hid-mcp2221.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e61dd039354b..de8b988f4a48 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -95,6 +95,8 @@ struct mcp2221 {
>  	struct mutex lock;
>  	struct completion wait_in_report;
>  	struct delayed_work init_work;
> +	spinlock_t init_work_lock;
> +	bool removing;
>  	u8 *rxbuf;
>  	u8 txbuf[64];
>  	int rxbuf_idx;
> @@ -922,6 +924,14 @@ static void mcp2221_hid_unregister(void *ptr)
>  /* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
>  static void mcp2221_remove(struct hid_device *hdev)
>  {
> +	struct mcp2221 *mcp = hid_get_drvdata(hdev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mcp->init_work_lock, flags);
> +	mcp->removing = true;
> +	spin_unlock_irqrestore(&mcp->init_work_lock, flags);
> +
> +	cancel_delayed_work_sync(&mcp->init_work);

Actually, given that the only re-submission of this work is from the
work item itself, I wonder if I really need the boolean and the
spinlock. cancel_delayed_work_sync() might already prevent a
resubmission by itself as it does in cancel_work_sync().

Cheers,
Benjamin

>  }
>  
>  #if IS_REACHABLE(CONFIG_IIO)
> @@ -1040,6 +1050,7 @@ static void mcp_init_work(struct work_struct *work)
>  	struct mcp2221_iio *data;
>  	static int retries = 5;
>  	int ret, num_channels;
> +	unsigned long flags;
>  
>  	hid_hw_power(mcp->hdev, PM_HINT_FULLON);
>  	mutex_lock(&mcp->lock);
> @@ -1090,7 +1101,10 @@ static void mcp_init_work(struct work_struct *work)
>  		return;
>  
>  	/* Device is not ready to read SRAM or FLASH data, try again */
> -	schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
> +	spin_lock_irqsave(&mcp->init_work_lock, flags);
> +	if (!mcp->removing)
> +		schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
> +	spin_unlock_irqrestore(&mcp->init_work_lock, flags);
>  }
>  #endif
>  
> @@ -1131,6 +1145,7 @@ static int mcp2221_probe(struct hid_device *hdev,
>  	}
>  
>  	mutex_init(&mcp->lock);
> +	spin_lock_init(&mcp->init_work_lock);
>  	init_completion(&mcp->wait_in_report);
>  	hid_set_drvdata(hdev, mcp);
>  	mcp->hdev = hdev;
> 
> ---
> base-commit: d883fd110dc17308a1506c5bf17e00ce9fe7b2a2
> change-id: 20230215-wip-mcp2221-979d4115efb5
> 
> Best regards,
> -- 
> Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-15 17:16 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-15 16:48 [PATCH] HID: mcp-2221: prevent UAF in delayed work Benjamin Tissoires via B4 Submission Endpoint
2023-02-15 16:48 ` Benjamin Tissoires
2023-02-15 17:15 ` Benjamin Tissoires

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.