Re: [PATCH] debugfs: Fix module state check condition
Hello, Greg, - Original Message - > From: "Greg KH" > Subject: Re: [PATCH] debugfs: Fix module state check condition > ...skip... > > A customer which has reported this issue replied with a test result: > > > > > I ran the same test. > > > Started ib_write_bw traffic and started watch command to read RoCE > > > stats : watch -d -n 1 "cat /sys/kernel/debug/bnxt_re/bnxt_re0/info". > > > While the command is running, unloaded roce driver and I did not > > > observe the call trace that was seen earlier. > > Having this info, that this was affecting a user, would have been good > in the original changelog info, otherwise this just looked like a code > cleanup patch to me. Thank you, Greg. Makes sense indeed, I will pay attention to this next time(s). oh so many little but important details to know and follow... > I'll go queue this up now, thanks. > > greg k-h Best regards, Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
Re: [PATCH] debugfs: Fix module state check condition
Hello, Greg, all, - Original Message - > From: "Greg KH" > Subject: Re: [PATCH] debugfs: Fix module state check condition > ...skip... > > It's in my queue, but bugs you can only trigger while root are a bit > lower on the priority list :) Oh, apologies. I really thought this has been somehow lost/slipped. Thank you for the reply and a confirmation. /me stops bothering people. > thanks, > > greg k-h > > Regards, Vladis
Re: [PATCH] debugfs: Fix module state check condition
Hello, Dear maintainers, could you please look at the above patch, that previously was sent during a merge window? A customer which has reported this issue replied with a test result: > I ran the same test. > Started ib_write_bw traffic and started watch command to read RoCE > stats : watch -d -n 1 "cat /sys/kernel/debug/bnxt_re/bnxt_re0/info". > While the command is running, unloaded roce driver and I did not > observe the call trace that was seen earlier. Regards, Vladis
[PATCH] debugfs: Fix module state check condition
The '#ifdef MODULE' check in the original commit does not work as intended.
The code under the check is not built at all if CONFIG_DEBUG_FS=y. Fix this
by using a correct check.
Fixes: 275678e7a9be ("debugfs: Check module state before warning in
{full/open}_proxy_open()")
Signed-off-by: Vladis Dronov
---
fs/debugfs/file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index b167d2d02148..a768a09430c3 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -177,7 +177,7 @@ static int open_proxy_open(struct inode *inode, struct file
*filp)
goto out;
if (!fops_get(real_fops)) {
-#ifdef MODULE
+#ifdef CONFIG_MODULES
if (real_fops->owner &&
real_fops->owner->state == MODULE_STATE_GOING)
goto out;
@@ -312,7 +312,7 @@ static int full_proxy_open(struct inode *inode, struct file
*filp)
goto out;
if (!fops_get(real_fops)) {
-#ifdef MODULE
+#ifdef CONFIG_MODULES
if (real_fops->owner &&
real_fops->owner->state == MODULE_STATE_GOING)
goto out;
--
2.26.2
Re: [PATCH] powerpc: fix function annotations to avoid section mismatch warnings with gcc-10
Hello, Michael,
- Original Message -
> From: "Michael Ellerman"
> Subject: Re: [PATCH] powerpc: fix function annotations to avoid section
> mismatch warnings with gcc-10
>
...
> >> > So what changed? These functions were inlined with older compilers, but
> >> > not anymore?
> >>
> >> Yes, exactly. Gcc-10 does not inline them anymore. If this is because of
> >> my
> >> build system, this can happen to others also.
> >>
> >> The same thing was fixed by Linus in e99332e7b4cd ("gcc-10: mark more
> >> functions
> >> __init to avoid section mismatch warnings").
> >
> > It sounds like this is part of "-finline-functions was retuned" on
> > <https://gcc.gnu.org/gcc-10/changes.html>? So everyone should see it
> > (no matter what config or build system), and it is a good thing too :-)
>
> I haven't seen it in my GCC 10 builds, so there must be some other
> subtlety. Probably it depends on details of the .config.
>
I've just had this building the latest upstream for the ppc64le with a
derivative
of the RHEL-8 config. This can probably be a compiler/linker setting, like -O2
versus -O3.
> cheers
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
Re: [PATCH] powerpc: fix function annotations to avoid section mismatch warnings with gcc-10
Hello,
- Original Message -
> From: "Segher Boessenkool"
> To: "Vladis Dronov"
> Cc: linuxppc-...@lists.ozlabs.org, "Aneesh Kumar K . V"
> , linux-kernel@vger.kernel.org,
> "Paul Mackerras"
> Sent: Wednesday, July 29, 2020 4:49:49 PM
> Subject: Re: [PATCH] powerpc: fix function annotations to avoid section
> mismatch warnings with gcc-10
>
> On Wed, Jul 29, 2020 at 03:37:41PM +0200, Vladis Dronov wrote:
> > Certain warnings are emitted for powerpc code when building with a gcc-10
> > toolset:
> >
> > WARNING: modpost: vmlinux.o(.text.unlikely+0x377c): Section mismatch in
> > reference from the function remove_pmd_table() to the function
> > .meminit.text:split_kernel_mapping()
> > The function remove_pmd_table() references
> > the function __meminit split_kernel_mapping().
> > This is often because remove_pmd_table lacks a __meminit
> > annotation or the annotation of split_kernel_mapping is wrong.
> >
> > Add the appropriate __init and __meminit annotations to make modpost not
> > complain. In all the cases there are just a single callsite from another
> > __init or __meminit function:
> >
> > __meminit remove_pagetable() -> remove_pud_table() -> remove_pmd_table()
> > __init prom_init() -> setup_secure_guest()
> > __init xive_spapr_init() -> xive_spapr_disabled()
>
> So what changed? These functions were inlined with older compilers, but
> not anymore?
Yes, exactly. Gcc-10 does not inline them anymore. If this is because of my
build system, this can happen to others also.
The same thing was fixed by Linus in e99332e7b4cd ("gcc-10: mark more functions
__init to avoid section mismatch warnings").
>
> Segher
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
[PATCH] powerpc: fix function annotations to avoid section mismatch warnings with gcc-10
Certain warnings are emitted for powerpc code when building with a gcc-10
toolset:
WARNING: modpost: vmlinux.o(.text.unlikely+0x377c): Section mismatch in
reference from the function remove_pmd_table() to the function
.meminit.text:split_kernel_mapping()
The function remove_pmd_table() references
the function __meminit split_kernel_mapping().
This is often because remove_pmd_table lacks a __meminit
annotation or the annotation of split_kernel_mapping is wrong.
Add the appropriate __init and __meminit annotations to make modpost not
complain. In all the cases there are just a single callsite from another
__init or __meminit function:
__meminit remove_pagetable() -> remove_pud_table() -> remove_pmd_table()
__init prom_init() -> setup_secure_guest()
__init xive_spapr_init() -> xive_spapr_disabled()
Signed-off-by: Vladis Dronov
---
arch/powerpc/kernel/prom_init.c | 4 ++--
arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++--
arch/powerpc/sysdev/xive/spapr.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 90c604d00b7d..f6ca7f450361 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -3262,7 +3262,7 @@ static int enter_secure_mode(unsigned long kbase,
unsigned long fdt)
/*
* Call the Ultravisor to transfer us to secure memory if we have an ESM blob.
*/
-static void setup_secure_guest(unsigned long kbase, unsigned long fdt)
+static void __init setup_secure_guest(unsigned long kbase, unsigned long fdt)
{
int ret;
@@ -3292,7 +3292,7 @@ static void setup_secure_guest(unsigned long kbase,
unsigned long fdt)
}
}
#else
-static void setup_secure_guest(unsigned long kbase, unsigned long fdt)
+static void __init setup_secure_guest(unsigned long kbase, unsigned long fdt)
{
}
#endif /* CONFIG_PPC_SVM */
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c
b/arch/powerpc/mm/book3s64/radix_pgtable.c
index bb00e0cba119..b868c07110e3 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -800,7 +800,7 @@ static void __meminit split_kernel_mapping(unsigned long
addr, unsigned long end
pte_clear(_mm, addr, pte);
}
-static void remove_pmd_table(pmd_t *pmd_start, unsigned long addr,
+static void __meminit remove_pmd_table(pmd_t *pmd_start, unsigned long addr,
unsigned long end)
{
unsigned long next;
@@ -825,7 +825,7 @@ static void remove_pmd_table(pmd_t *pmd_start, unsigned
long addr,
}
}
-static void remove_pud_table(pud_t *pud_start, unsigned long addr,
+static void __meminit remove_pud_table(pud_t *pud_start, unsigned long addr,
unsigned long end)
{
unsigned long next;
diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
index f0551a2be9df..1e3674d7ea7b 100644
--- a/arch/powerpc/sysdev/xive/spapr.c
+++ b/arch/powerpc/sysdev/xive/spapr.c
@@ -768,7 +768,7 @@ static const u8 *get_vec5_feature(unsigned int index)
return vec5 + index;
}
-static bool xive_spapr_disabled(void)
+static bool __init xive_spapr_disabled(void)
{
const u8 *vec5_xive;
--
2.26.2
Re: [PATCH v5.3-rc2] Bluetooth: hci_uart: check for missing tty operations
Hello, Greg, all, I've just double-checked your backports, indeed, they are fine. Check for operations is not added for protocols which does not use these operations. Thanks! Best regards, Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer - Original Message - > From: "Greg KH" > To: "Vladis Dronov" > Cc: "Sasha Levin" , "Marcel Holtmann" > , torva...@linux-foundation.org, > linux-kernel@vger.kernel.org, linux-blueto...@vger.kernel.org, > sta...@vger.kernel.org > Sent: Thursday, August 1, 2019 4:06:39 PM > Subject: Re: [PATCH v5.3-rc2] Bluetooth: hci_uart: check for missing tty > operations > > On Thu, Aug 01, 2019 at 09:55:55AM -0400, Vladis Dronov wrote: > > Thank you, Greg! > > > > I've just noticed the patch landed in the upstream and was going to start > > stable > > backports, but it appeared you've already done this. > > Verifying that I got the 4.4.y and 4.9.y and 4.14.y backports done > properly would be good, as I took a guess at them :) > > thanks, > > greg k-h
Re: [PATCH v5.3-rc2] Bluetooth: hci_uart: check for missing tty operations
Thank you, Greg!
I've just noticed the patch landed in the upstream and was going to start stable
backports, but it appeared you've already done this.
So, not only automated mailers are slow :)
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
- Original Message -
> From: "Greg KH"
> To: "Sasha Levin"
> Cc: "Marcel Holtmann" , "Vladis Dronov"
> , torva...@linux-foundation.org,
> linux-kernel@vger.kernel.org, linux-blueto...@vger.kernel.org,
> sta...@vger.kernel.org
> Sent: Thursday, August 1, 2019 3:50:44 PM
> Subject: Re: [PATCH v5.3-rc2] Bluetooth: hci_uart: check for missing tty
> operations
>
> On Thu, Aug 01, 2019 at 01:31:31PM +, Sasha Levin wrote:
> > Hi,
> >
> > [This is an automated email]
> >
> > This commit has been processed because it contains a "Fixes:" tag,
> > fixing commit: .
> >
> > The bot has tested the following trees: v5.2.4, v5.1.21, v4.19.62,
> > v4.14.134, v4.9.186, v4.4.186.
> >
> > v5.2.4: Build OK!
> > v5.1.21: Build OK!
> > v4.19.62: Build OK!
> > v4.14.134: Failed to apply! Possible dependencies:
> > 25a13e382de2 ("bluetooth: hci_qca: Replace GFP_ATOMIC with GFP_KERNEL")
> >
> > v4.9.186: Failed to apply! Possible dependencies:
> > 25a13e382de2 ("bluetooth: hci_qca: Replace GFP_ATOMIC with GFP_KERNEL")
> >
> > v4.4.186: Failed to apply! Possible dependencies:
> > 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
> > 25a13e382de2 ("bluetooth: hci_qca: Replace GFP_ATOMIC with GFP_KERNEL")
> > 395174bb07c1 ("Bluetooth: hci_uart: Add Intel/AG6xx support")
> > 9e69130c4efc ("Bluetooth: hci_uart: Add Nokia Protocol identifier")
> >
> >
> > NOTE: The patch will not be queued to stable trees until it is upstream.
> >
> > How should we proceed with this patch?
>
> Already fixed up by hand and queued up, your automated email is a bit
> slow :)
>
> greg k-h
>
[PATCH v3] Bluetooth: hci_uart: check for missing tty operations
Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
functions which are called by the certain HCI UART protocols (hci_ath,
hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
or directly. This leads to an execution at NULL and can be triggered by
an unprivileged user. Fix this by adding a helper function and a check
for the missing tty operations in the protocols code.
This fixes CVE-2019-10207. The Fixes: lines list commits where calls to
tiocm[gs]et() or hci_uart_set_flow_control() were added to the HCI UART
protocols.
Link:
https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
Reported-by: syzbot+79337b501d6aa974d...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org # v2.6.36+
Fixes: b3190df62861 ("Bluetooth: Support for Atheros AR300x serial chip")
Fixes: 118612fb9165 ("Bluetooth: hci_bcm: Add suspend/resume PM functions")
Fixes: ff2895592f0f ("Bluetooth: hci_intel: Add Intel baudrate configuration
support")
Fixes: 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
Fixes: fa9ad876b8e0 ("Bluetooth: hci_qca: Add support for Qualcomm Bluetooth
chip wcn3990")
Signed-off-by: Vladis Dronov
---
out-of-commit-message-note:
it is possible that a HCI UART protocol uses serial device's
operations and not a tty ones. i made hci_uart_has_flow_control() to
check this also, hence the name. serial device's code checks if
the needed operations are present itself.
i believe, hci_h5 does not need this check, as it uses serial device
functions only (as of now):
serdev_device_set_flow_control(h5->hu->serdev, false);
serdev_device_set_baudrate(h5->hu->serdev, 115200);
drivers/bluetooth/hci_ath.c | 3 +++
drivers/bluetooth/hci_bcm.c | 3 +++
drivers/bluetooth/hci_intel.c | 3 +++
drivers/bluetooth/hci_ldisc.c | 7 +++
drivers/bluetooth/hci_mrvl.c | 3 +++
drivers/bluetooth/hci_qca.c | 3 +++
drivers/bluetooth/hci_uart.h | 1 +
7 files changed, 23 insertions(+)
diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index a55be205b91a..dbfe34664633 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -98,6 +98,9 @@ static int ath_open(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
ath = kzalloc(sizeof(*ath), GFP_KERNEL);
if (!ath)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 8905ad2edde7..ae2624fce913 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -406,6 +406,9 @@ static int bcm_open(struct hci_uart *hu)
bt_dev_dbg(hu->hdev, "hu %p", hu);
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
if (!bcm)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 207bae5e0d46..31f25153087d 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -391,6 +391,9 @@ static int intel_open(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
intel = kzalloc(sizeof(*intel), GFP_KERNEL);
if (!intel)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 8950e07889fe..11049e2336f3 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -292,6 +292,13 @@ static int hci_uart_send_frame(struct hci_dev *hdev,
struct sk_buff *skb)
return 0;
}
+/* Check the underlying device or tty has flow control support */
+bool hci_uart_has_flow_control(struct hci_uart *hu)
+{
+ return hu->serdev ||
+ (hu->tty->driver->ops->tiocmget &&
hu->tty->driver->ops->tiocmset);
+}
+
/* Flow control or un-flow control the device */
void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
{
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index f98e5cc343b2..fbc3f7c3a5c7 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -59,6 +59,9 @@ static int mrvl_open(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
mrvl = kzalloc(sizeof(*mrvl), GFP_KERNEL);
if (!mrvl)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 9a5c9c1f9484..82a0a3691a63 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -473,6 +473,9 @@ static int qca_open(struct hci_uart *hu)
BT_DBG("hu %p qca_open", hu);
+ if (!hci_uart_has_flow_control(hu))
+ re
[PATCH v2] Bluetooth: hci_ldisc: check for missing tty operations
Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
functions which are called by the certain HCI UART protocols (hci_ath,
hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
or directly. This leads to an execution at NULL and can be triggered by
an unprivileged user. Fix this by adding a check for the missing tty
operations the same way it is done for write().
This fixes CVE-2019-10207. The Fixes: lines list commits where calls to
tiocm[gs]et() or hci_uart_set_flow_control() were added to the HCI UART
protocols.
Link:
https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
Reported-by: syzbot+79337b501d6aa974d...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org # v2.6.36+
Fixes: c19483cc5e56 ("bluetooth: Fix missing NULL check")
Fixes: b3190df62861 ("Bluetooth: Support for Atheros AR300x serial chip")
Fixes: 118612fb9165 ("Bluetooth: hci_bcm: Add suspend/resume PM functions")
Fixes: ff2895592f0f ("Bluetooth: hci_intel: Add Intel baudrate configuration
support")
Fixes: 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
Fixes: fa9ad876b8e0 ("Bluetooth: hci_qca: Add support for Qualcomm Bluetooth
chip wcn3990")
Signed-off-by: Vladis Dronov
---
out-of-commit-message-note:
I believe, this is a good location for the check. This way we protect protocols
which does not call tiocm[gs]et() or hci_uart_set_flow_control() but may
change to call them in the future.
Also we do not need hci_uart_has_tiocm_support() helper now.
drivers/bluetooth/hci_ldisc.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index c84f985f348d..4a85c51d0307 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -459,10 +459,11 @@ static int hci_uart_tty_open(struct tty_struct *tty)
BT_DBG("tty %p", tty);
- /* Error if the tty has no write op instead of leaving an exploitable
-* hole
+ /* Error if the tty has no write or tiocm[gs]et ops instead of leaving
+* an exploitable hole
*/
- if (tty->ops->write == NULL)
+ if (tty->ops->write == NULL || tty->ops->tiocmget == NULL ||
+ tty->ops->tiocmset == NULL)
return -EOPNOTSUPP;
hu = kzalloc(sizeof(struct hci_uart), GFP_KERNEL);
--
2.21.0
Re: [PATCH] Bluetooth: hci_uart: check for missing tty operations in protocol handlers
Hello, Marcel,
> why is this one hidden behind CONFIG_PM? The general baud rate changes are
> independent of runtime power management support.
hci_bcm calls hci_uart_set_flow_control() only from functions hidden behind
#ifdef-CONFIG_PM (surely this can change in the future), and so without
CONFIG_PM set it cannot hit the bug (as of now). So I've hidden the check
for tiocm[gs]et() behind #ifdef-CONFIG_PM too.
If you tell me it is better to remove this #ifdef, I'll remove it.
> And I would introduce a bool hci_uart_has_tiocm_support(struct hci_uart *)
> helper.
Great, I will add it to the v2 fix. I guess a good place for it is hci_ldisc.c,
near hci_uart_set_flow_control(), isn't it?
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
- Original Message -
> From: "Marcel Holtmann"
> To: "Vladis Dronov"
> Cc: "Johan Hedberg" ,
> linux-blueto...@vger.kernel.org, linux-kernel@vger.kernel.org, "Suraj
> Sumangala" , "Frederic Danis"
> , "Loic Poulain"
> , "Balakrishna Godavarthi" ,
> syzkal...@googlegroups.com
> Sent: Thursday, July 25, 2019 2:59:42 PM
> Subject: Re: [PATCH] Bluetooth: hci_uart: check for missing tty operations in
> protocol handlers
>
> Hi Vladis,
>
> > Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
> > functions which are called by the certain HCI UART protocols (hci_ath,
> > hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
> > or directly. This leads to an execution at NULL and can be triggered by
> > an unprivileged user. Fix this by adding a check for the missing tty
> > operations to the protocols which use them.
> >
> > This fixes CVE-2019-10207.
> >
> > Link:
> > https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
> > Reported-by: syzbot+79337b501d6aa974d...@syzkaller.appspotmail.com
> > Cc: sta...@vger.kernel.org # v2.6.36+
> > Fixes: b3190df62861 ("Bluetooth: Support for Atheros AR300x serial chip")
> > Fixes: 118612fb9165 ("Bluetooth: hci_bcm: Add suspend/resume PM functions")
> > Fixes: ff2895592f0f ("Bluetooth: hci_intel: Add Intel baudrate
> > configuration support")
> > Fixes: 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
> > Fixes: fa9ad876b8e0 ("Bluetooth: hci_qca: Add support for Qualcomm
> > Bluetooth chip wcn3990")
> > Signed-off-by: Vladis Dronov
> > ---
> > drivers/bluetooth/hci_ath.c | 3 +++
> > drivers/bluetooth/hci_bcm.c | 5 +
> > drivers/bluetooth/hci_intel.c | 3 +++
> > drivers/bluetooth/hci_mrvl.c | 3 +++
> > drivers/bluetooth/hci_qca.c | 4
> > 5 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> > index a55be205b91a..99df8a13e47e 100644
> > --- a/drivers/bluetooth/hci_ath.c
> > +++ b/drivers/bluetooth/hci_ath.c
> > @@ -98,6 +98,9 @@ static int ath_open(struct hci_uart *hu)
> >
> > BT_DBG("hu %p", hu);
> >
> > + if (!hu->tty->driver->ops->tiocmget || !hu->tty->driver->ops->tiocmset)
> > + return -ENOTSUPP;
> > +
> > ath = kzalloc(sizeof(*ath), GFP_KERNEL);
> > if (!ath)
> > return -ENOMEM;
> > diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> > index 8905ad2edde7..8c3e09cc341c 100644
> > --- a/drivers/bluetooth/hci_bcm.c
> > +++ b/drivers/bluetooth/hci_bcm.c
> > @@ -406,6 +406,11 @@ static int bcm_open(struct hci_uart *hu)
> >
> > bt_dev_dbg(hu->hdev, "hu %p", hu);
> >
> > +#ifdef CONFIG_PM
> > + if (!hu->tty->driver->ops->tiocmget || !hu->tty->driver->ops->tiocmset)
> > + return -ENOTSUPP;
> > +#endif
> > +
>
> why is this one hidden behind CONFIG_PM? The general baud rate changes are
> independent of runtime power management support.
>
> And I would introduce a bool hci_uart_has_tiocm_support(struct hci_uart *)
> helper.
>
> Regards
>
> Marcel
[PATCH] Bluetooth: hci_uart: check for missing tty operations in protocol handlers
Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
functions which are called by the certain HCI UART protocols (hci_ath,
hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
or directly. This leads to an execution at NULL and can be triggered by
an unprivileged user. Fix this by adding a check for the missing tty
operations to the protocols which use them.
This fixes CVE-2019-10207.
Link:
https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
Reported-by: syzbot+79337b501d6aa974d...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org # v2.6.36+
Fixes: b3190df62861 ("Bluetooth: Support for Atheros AR300x serial chip")
Fixes: 118612fb9165 ("Bluetooth: hci_bcm: Add suspend/resume PM functions")
Fixes: ff2895592f0f ("Bluetooth: hci_intel: Add Intel baudrate configuration
support")
Fixes: 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
Fixes: fa9ad876b8e0 ("Bluetooth: hci_qca: Add support for Qualcomm Bluetooth
chip wcn3990")
Signed-off-by: Vladis Dronov
---
drivers/bluetooth/hci_ath.c | 3 +++
drivers/bluetooth/hci_bcm.c | 5 +
drivers/bluetooth/hci_intel.c | 3 +++
drivers/bluetooth/hci_mrvl.c | 3 +++
drivers/bluetooth/hci_qca.c | 4
5 files changed, 18 insertions(+)
diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index a55be205b91a..99df8a13e47e 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -98,6 +98,9 @@ static int ath_open(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ if (!hu->tty->driver->ops->tiocmget || !hu->tty->driver->ops->tiocmset)
+ return -ENOTSUPP;
+
ath = kzalloc(sizeof(*ath), GFP_KERNEL);
if (!ath)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 8905ad2edde7..8c3e09cc341c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -406,6 +406,11 @@ static int bcm_open(struct hci_uart *hu)
bt_dev_dbg(hu->hdev, "hu %p", hu);
+#ifdef CONFIG_PM
+ if (!hu->tty->driver->ops->tiocmget || !hu->tty->driver->ops->tiocmset)
+ return -ENOTSUPP;
+#endif
+
bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
if (!bcm)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index 207bae5e0d46..a314882a1cac 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -391,6 +391,9 @@ static int intel_open(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ if (!hu->tty->driver->ops->tiocmget || !hu->tty->driver->ops->tiocmset)
+ return -ENOTSUPP;
+
intel = kzalloc(sizeof(*intel), GFP_KERNEL);
if (!intel)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index f98e5cc343b2..4ee27c8d8679 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -59,6 +59,9 @@ static int mrvl_open(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ if (!hu->tty->driver->ops->tiocmget || !hu->tty->driver->ops->tiocmset)
+ return -ENOTSUPP;
+
mrvl = kzalloc(sizeof(*mrvl), GFP_KERNEL);
if (!mrvl)
return -ENOMEM;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 9a5c9c1f9484..a65c202e8995 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
@@ -473,6 +474,9 @@ static int qca_open(struct hci_uart *hu)
BT_DBG("hu %p qca_open", hu);
+ if (!hu->tty->driver->ops->tiocmget || !hu->tty->driver->ops->tiocmset)
+ return -ENOTSUPP;
+
qca = kzalloc(sizeof(struct qca_data), GFP_KERNEL);
if (!qca)
return -ENOMEM;
--
2.20.1
Re: [PATCH] Bluetooth: hci_ldisc: check for missing tty operations
Hello, Marcel, I totally agree, the same came to my mind some time after sending the patch. Let me send a v2 in a while with drivers checking for needed tty operations presence. Best regards, Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer - Original Message - > From: "Marcel Holtmann" > To: "Vladis Dronov" > Cc: "Johan Hedberg" , > linux-blueto...@vger.kernel.org, linux-kernel@vger.kernel.org, > syzbot+79337b501d6aa974d...@syzkaller.appspotmail.com, > sta...@vger.kernel.org, "Loic Poulain" > , "Ilya Faenson" > Sent: Saturday, July 6, 2019 12:35:39 PM > Subject: Re: [PATCH] Bluetooth: hci_ldisc: check for missing tty operations > > Hi Vladis, > > > Certain ttys lack operations (eg: pty_unix98_ops) which still can be > > called by certain hci uart proto (eg: mrvl, ath). Currently this leads > > to execution at address 0x0. Fix this by adding checks for missing tty > > operations. > > so I really prefer that we just fail setting the line discipline. These > drivers need to check that the underlying transport has all the operations > available they need. We can not just ignore them. > > Regards > > Marcel
[PATCH] Bluetooth: hci_ldisc: check for missing tty operations
Certain ttys lack operations (eg: pty_unix98_ops) which still can be
called by certain hci uart proto (eg: mrvl, ath). Currently this leads
to execution at address 0x0. Fix this by adding checks for missing tty
operations.
Link:
https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
Reported-by: syzbot+79337b501d6aa974d...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org # v2.6.39+
Signed-off-by: Vladis Dronov
---
drivers/bluetooth/hci_ath.c | 7 -
drivers/bluetooth/hci_ldisc.c | 58 ---
2 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index a55be205b91a..92d9bded5880 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -49,7 +49,12 @@ struct ath_vendor_cmd {
static int ath_wakeup_ar3k(struct tty_struct *tty)
{
- int status = tty->driver->ops->tiocmget(tty);
+ int status;
+
+ if (!(tty->driver->ops->tiocmget && tty->driver->ops->tiocmset))
+ return TIOCM_CTS;
+
+ status = tty->driver->ops->tiocmget(tty);
if (status & TIOCM_CTS)
return status;
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index c84f985f348d..29b4970f9bca 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -307,32 +307,40 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool
enable)
BT_DBG("Disabling hardware flow control: %s",
status ? "failed" : "success");
- /* Clear RTS to prevent the device from sending */
- /* Most UARTs need OUT2 to enable interrupts */
- status = tty->driver->ops->tiocmget(tty);
- BT_DBG("Current tiocm 0x%x", status);
-
- set &= ~(TIOCM_OUT2 | TIOCM_RTS);
- clear = ~set;
- set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
- TIOCM_OUT2 | TIOCM_LOOP;
- clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
-TIOCM_OUT2 | TIOCM_LOOP;
- status = tty->driver->ops->tiocmset(tty, set, clear);
- BT_DBG("Clearing RTS: %s", status ? "failed" : "success");
+ if (tty->driver->ops->tiocmget && tty->driver->ops->tiocmset) {
+ /* Clear RTS to prevent the device from sending */
+ /* Most UARTs need OUT2 to enable interrupts */
+ status = tty->driver->ops->tiocmget(tty);
+ BT_DBG("Current tiocm 0x%x", status);
+
+ set &= ~(TIOCM_OUT2 | TIOCM_RTS);
+ clear = ~set;
+ set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+ TIOCM_OUT2 | TIOCM_LOOP;
+ clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+TIOCM_OUT2 | TIOCM_LOOP;
+ status = tty->driver->ops->tiocmset(tty, set, clear);
+ BT_DBG("Clearing RTS: %s",
+ status ? "failed" : "success");
+ } else
+ BT_DBG("Terminal RTS control is not present");
} else {
- /* Set RTS to allow the device to send again */
- status = tty->driver->ops->tiocmget(tty);
- BT_DBG("Current tiocm 0x%x", status);
-
- set |= (TIOCM_OUT2 | TIOCM_RTS);
- clear = ~set;
- set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
- TIOCM_OUT2 | TIOCM_LOOP;
- clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
-TIOCM_OUT2 | TIOCM_LOOP;
- status = tty->driver->ops->tiocmset(tty, set, clear);
- BT_DBG("Setting RTS: %s", status ? "failed" : "success");
+ if (tty->driver->ops->tiocmget && tty->driver->ops->tiocmset) {
+ /* Set RTS to allow the device to send again */
+ status = tty->driver->ops->tiocmget(tty);
+ BT_DBG("Current tiocm 0x%x", status);
+
+ set |= (TIOCM_OUT2 | TIOCM_RTS);
+ clear = ~set;
+ set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+ TIOCM_OUT2 | TIOCM_LOOP;
+ clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+TIOCM_OUT2 | TIOCM_LOOP;
+ status = tty->driver->ops->tiocmset(tty, set, clear);
+ BT_DBG("Setting RTS: %s",
+ status ? "failed" : "success");
+ } else
+ BT_DBG("Terminal RTS control is not present");
/* Re-enable hardware flow control */
ktermios = tty->termios;
--
2.20.1
Re: [PATCH v2] HID: debug: fix the ring buffer implementation
> > I still think that > > > > __set_current_state(TASK_RUNNING); > > > > will look a bit better, but this is really minor. > > Would you mind sending a v3 with this change? I'll apply it ASAP. Done, please, see inbox. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security | Senior Software Engineer
[PATCH v3] HID: debug: fix the ring buffer implementation
Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
is strange allowing lost or corrupted data. After commit 717adfdaf147
("HID: debug: check length before copy_to_user()") it is possible to enter
an infinite loop in hid_debug_events_read() by providing 0 as count, this
locks up a system. Fix this by rewriting the ring buffer implementation
with kfifo and simplify the code.
This fixes CVE-2019-3819.
v2: fix an execution logic and add a comment
v3: use __set_current_state() instead of set_current_state()
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1669187
Cc: sta...@vger.kernel.org # v4.18+
Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
Fixes: 717adfdaf147 ("HID: debug: check length before copy_to_user()")
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 116 ++
include/linux/hid-debug.h | 9 ++-
2 files changed, 47 insertions(+), 78 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index c530476edba6..08870c909268 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -30,6 +30,7 @@
#include
#include
+#include
#include
#include
#include
@@ -661,17 +662,12 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
/* enqueue string to 'events' ring buffer */
void hid_debug_event(struct hid_device *hdev, char *buf)
{
- unsigned i;
struct hid_debug_list *list;
unsigned long flags;
spin_lock_irqsave(>debug_list_lock, flags);
- list_for_each_entry(list, >debug_list, node) {
- for (i = 0; buf[i]; i++)
- list->hid_debug_buf[(list->tail + i) %
HID_DEBUG_BUFSIZE] =
- buf[i];
- list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
-}
+ list_for_each_entry(list, >debug_list, node)
+ kfifo_in(>hid_debug_fifo, buf, strlen(buf));
spin_unlock_irqrestore(>debug_list_lock, flags);
wake_up_interruptible(>debug_wait);
@@ -722,8 +718,7 @@ void hid_dump_input(struct hid_device *hdev, struct
hid_usage *usage, __s32 valu
hid_debug_event(hdev, buf);
kfree(buf);
-wake_up_interruptible(>debug_wait);
-
+ wake_up_interruptible(>debug_wait);
}
EXPORT_SYMBOL_GPL(hid_dump_input);
@@ -1083,8 +1078,8 @@ static int hid_debug_events_open(struct inode *inode,
struct file *file)
goto out;
}
- if (!(list->hid_debug_buf = kzalloc(HID_DEBUG_BUFSIZE, GFP_KERNEL))) {
- err = -ENOMEM;
+ err = kfifo_alloc(>hid_debug_fifo, HID_DEBUG_FIFOSIZE,
GFP_KERNEL);
+ if (err) {
kfree(list);
goto out;
}
@@ -1104,77 +1099,57 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
size_t count, loff_t *ppos)
{
struct hid_debug_list *list = file->private_data;
- int ret = 0, len;
+ int ret = 0, copied;
DECLARE_WAITQUEUE(wait, current);
mutex_lock(>read_mutex);
- while (ret == 0) {
- if (list->head == list->tail) {
- add_wait_queue(>hdev->debug_wait, );
- set_current_state(TASK_INTERRUPTIBLE);
-
- while (list->head == list->tail) {
- if (file->f_flags & O_NONBLOCK) {
- ret = -EAGAIN;
- break;
- }
- if (signal_pending(current)) {
- ret = -ERESTARTSYS;
- break;
- }
-
- if (!list->hdev || !list->hdev->debug) {
- ret = -EIO;
- set_current_state(TASK_RUNNING);
- goto out;
- }
-
- /* allow O_NONBLOCK from other threads */
- mutex_unlock(>read_mutex);
- schedule();
- mutex_lock(>read_mutex);
- set_current_state(TASK_INTERRUPTIBLE);
- }
-
- set_current_state(TASK_RUNNING);
- remove_wait_queue(>hdev->debug_wait, );
- }
-
- if (ret)
- goto out;
+ if (kfifo_is_empty(>hid_debug_fifo)) {
+ add_wait_queue(>hdev->debug_wait, );
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ while (kfifo_is_empty(>hid_debug_fifo)) {
+ if (file->f_flags & O_NONBLOCK) {
+
[PATCH v2] HID: debug: fix the ring buffer implementation
Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
is strange allowing lost or corrupted data. After commit 717adfdaf147
("HID: debug: check length before copy_to_user()") it is possible to enter
an infinite loop in hid_debug_events_read() by providing 0 as count, this
locks up a system. Fix this by rewriting the ring buffer implementation
with kfifo and simplify the code.
This fixes CVE-2019-3819.
v2: fix an execution logic and add a comment
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1669187
Cc: sta...@vger.kernel.org # v4.18+
Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
Fixes: 717adfdaf147 ("HID: debug: check length before copy_to_user()")
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 116 ++
include/linux/hid-debug.h | 9 ++-
2 files changed, 47 insertions(+), 78 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index c530476edba6..08870c909268 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -30,6 +30,7 @@
#include
#include
+#include
#include
#include
#include
@@ -661,17 +662,12 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
/* enqueue string to 'events' ring buffer */
void hid_debug_event(struct hid_device *hdev, char *buf)
{
- unsigned i;
struct hid_debug_list *list;
unsigned long flags;
spin_lock_irqsave(>debug_list_lock, flags);
- list_for_each_entry(list, >debug_list, node) {
- for (i = 0; buf[i]; i++)
- list->hid_debug_buf[(list->tail + i) %
HID_DEBUG_BUFSIZE] =
- buf[i];
- list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
-}
+ list_for_each_entry(list, >debug_list, node)
+ kfifo_in(>hid_debug_fifo, buf, strlen(buf));
spin_unlock_irqrestore(>debug_list_lock, flags);
wake_up_interruptible(>debug_wait);
@@ -722,8 +718,7 @@ void hid_dump_input(struct hid_device *hdev, struct
hid_usage *usage, __s32 valu
hid_debug_event(hdev, buf);
kfree(buf);
-wake_up_interruptible(>debug_wait);
-
+ wake_up_interruptible(>debug_wait);
}
EXPORT_SYMBOL_GPL(hid_dump_input);
@@ -1083,8 +1078,8 @@ static int hid_debug_events_open(struct inode *inode,
struct file *file)
goto out;
}
- if (!(list->hid_debug_buf = kzalloc(HID_DEBUG_BUFSIZE, GFP_KERNEL))) {
- err = -ENOMEM;
+ err = kfifo_alloc(>hid_debug_fifo, HID_DEBUG_FIFOSIZE,
GFP_KERNEL);
+ if (err) {
kfree(list);
goto out;
}
@@ -1104,77 +1099,57 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
size_t count, loff_t *ppos)
{
struct hid_debug_list *list = file->private_data;
- int ret = 0, len;
+ int ret = 0, copied;
DECLARE_WAITQUEUE(wait, current);
mutex_lock(>read_mutex);
- while (ret == 0) {
- if (list->head == list->tail) {
- add_wait_queue(>hdev->debug_wait, );
- set_current_state(TASK_INTERRUPTIBLE);
-
- while (list->head == list->tail) {
- if (file->f_flags & O_NONBLOCK) {
- ret = -EAGAIN;
- break;
- }
- if (signal_pending(current)) {
- ret = -ERESTARTSYS;
- break;
- }
-
- if (!list->hdev || !list->hdev->debug) {
- ret = -EIO;
- set_current_state(TASK_RUNNING);
- goto out;
- }
-
- /* allow O_NONBLOCK from other threads */
- mutex_unlock(>read_mutex);
- schedule();
- mutex_lock(>read_mutex);
- set_current_state(TASK_INTERRUPTIBLE);
- }
-
- set_current_state(TASK_RUNNING);
- remove_wait_queue(>hdev->debug_wait, );
- }
-
- if (ret)
- goto out;
+ if (kfifo_is_empty(>hid_debug_fifo)) {
+ add_wait_queue(>hdev->debug_wait, );
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ while (kfifo_is_empty(>hid_debug_fifo)) {
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+
Re: [PATCH] HID: debug: fix the ring buffer implementation
Hello, Oleg, all, Thanks much, Oleg, these are really valuable additions: > suspicious ;) if you add a comment the patch will be even better. Great, will do. > perhaps it make sense to move this check into the "if (kfifo_is_empty())" > block. Indeed, otherwise it kinda breaks the execution logic. > is kfifo_is_empty() == T really possible here? It looks like it is not. Reads are guarded by read_mutex and the only other code which touches hid_debug_fifo is writer. I will post v2 here soon. Lets see if it is ready for inclusion. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security | Senior Software Engineer
[PATCH] HID: debug: fix the ring buffer implementation
Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
is strange allowing lost or corrupted data. After commit 717adfdaf147
("HID: debug: check length before copy_to_user()") it is possible to enter
an infinite loop in hid_debug_events_read() by providing 0 as count, this
locks up a system. Fix this by rewriting the ring buffer implementation
with kfifo and simplify the code.
This fixes CVE-2019-3819.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1669187
Cc: sta...@vger.kernel.org # v4.16+
Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
Fixes: 717adfdaf147 ("HID: debug: check length before copy_to_user()")
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 116 ++
include/linux/hid-debug.h | 9 ++-
2 files changed, 47 insertions(+), 78 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index c530476edba6..08870c909268 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -30,6 +30,7 @@
#include
#include
+#include
#include
#include
#include
@@ -661,17 +662,12 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
/* enqueue string to 'events' ring buffer */
void hid_debug_event(struct hid_device *hdev, char *buf)
{
- unsigned i;
struct hid_debug_list *list;
unsigned long flags;
spin_lock_irqsave(>debug_list_lock, flags);
- list_for_each_entry(list, >debug_list, node) {
- for (i = 0; buf[i]; i++)
- list->hid_debug_buf[(list->tail + i) %
HID_DEBUG_BUFSIZE] =
- buf[i];
- list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
-}
+ list_for_each_entry(list, >debug_list, node)
+ kfifo_in(>hid_debug_fifo, buf, strlen(buf));
spin_unlock_irqrestore(>debug_list_lock, flags);
wake_up_interruptible(>debug_wait);
@@ -722,8 +718,7 @@ void hid_dump_input(struct hid_device *hdev, struct
hid_usage *usage, __s32 valu
hid_debug_event(hdev, buf);
kfree(buf);
-wake_up_interruptible(>debug_wait);
-
+ wake_up_interruptible(>debug_wait);
}
EXPORT_SYMBOL_GPL(hid_dump_input);
@@ -1083,8 +1078,8 @@ static int hid_debug_events_open(struct inode *inode,
struct file *file)
goto out;
}
- if (!(list->hid_debug_buf = kzalloc(HID_DEBUG_BUFSIZE, GFP_KERNEL))) {
- err = -ENOMEM;
+ err = kfifo_alloc(>hid_debug_fifo, HID_DEBUG_FIFOSIZE,
GFP_KERNEL);
+ if (err) {
kfree(list);
goto out;
}
@@ -1104,77 +1099,55 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
size_t count, loff_t *ppos)
{
struct hid_debug_list *list = file->private_data;
- int ret = 0, len;
+ int ret = 0, copied;
DECLARE_WAITQUEUE(wait, current);
mutex_lock(>read_mutex);
- while (ret == 0) {
- if (list->head == list->tail) {
- add_wait_queue(>hdev->debug_wait, );
- set_current_state(TASK_INTERRUPTIBLE);
-
- while (list->head == list->tail) {
- if (file->f_flags & O_NONBLOCK) {
- ret = -EAGAIN;
- break;
- }
- if (signal_pending(current)) {
- ret = -ERESTARTSYS;
- break;
- }
-
- if (!list->hdev || !list->hdev->debug) {
- ret = -EIO;
- set_current_state(TASK_RUNNING);
- goto out;
- }
-
- /* allow O_NONBLOCK from other threads */
- mutex_unlock(>read_mutex);
- schedule();
- mutex_lock(>read_mutex);
- set_current_state(TASK_INTERRUPTIBLE);
- }
-
- set_current_state(TASK_RUNNING);
- remove_wait_queue(>hdev->debug_wait, );
- }
-
- if (ret)
- goto out;
+ if (kfifo_is_empty(>hid_debug_fifo)) {
+ add_wait_queue(>hdev->debug_wait, );
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ while (kfifo_is_empty(>hid_debug_fifo)) {
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ break;
+ }
+
+
Re: [PATCH 0/3] HID: debug: fix the ring buffer implementation
Hello, Jiri, Thank you for the reply and your opinion. It appeared that my own implementation of a ring buffer was kind of "inventing a wheel", as "kfifo" is already is the kernel and it may work as a ring buffer quite well. I would like to rewrite my patchset and use kfifo instead in a new one. Please, ignore this my patchset and I'll try to submit v2 soon. This also will answer to "how was it tested" concern, as I believe, kfifo was quite tested. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer - Original Message - > From: "Jiri Kosina" > To: "Vladis Dronov" > Cc: "Benjamin Tissoires" , > linux-in...@vger.kernel.org, linux-kernel@vger.kernel.org > Sent: Friday, October 26, 2018 5:25:21 PM > Subject: Re: [PATCH 0/3] HID: debug: fix the ring buffer implementation > > On Wed, 3 Oct 2018, Vladis Dronov wrote: > > > This patchset is fixing some aspects of the ring buffer implementation in > > drivers/hid/hid-debug.c. This implementation has certain problem points: > > > > - it may stuck in an infinite loop > > - it may return corrupted data > > - a reader and a writer are not protected by spinlocks, which can lead to > > the corrupted data > > > > The suggested patchset is a new ring buffer implementation which overwrites > > the oldest data in case of an overflow. One can verify the suggested ring > > buffer implementation by fuzzing it with modified kernel and fuzzer-reader > > at: https://gist.github.com/nefigtut/33d56e3870b67493cc867344aed2a062 > > Vladis, > > thanks for cleaning it up. I actually like your rewrite quite a lot. > > Quick question -- how well was it tested in which scenarios? > > -- > Jiri Kosina > SUSE Labs
Re: [PATCH 0/3] HID: debug: fix the ring buffer implementation
Hello, Jiri, Thank you for the reply and your opinion. It appeared that my own implementation of a ring buffer was kind of "inventing a wheel", as "kfifo" is already is the kernel and it may work as a ring buffer quite well. I would like to rewrite my patchset and use kfifo instead in a new one. Please, ignore this my patchset and I'll try to submit v2 soon. This also will answer to "how was it tested" concern, as I believe, kfifo was quite tested. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer - Original Message - > From: "Jiri Kosina" > To: "Vladis Dronov" > Cc: "Benjamin Tissoires" , > linux-in...@vger.kernel.org, linux-kernel@vger.kernel.org > Sent: Friday, October 26, 2018 5:25:21 PM > Subject: Re: [PATCH 0/3] HID: debug: fix the ring buffer implementation > > On Wed, 3 Oct 2018, Vladis Dronov wrote: > > > This patchset is fixing some aspects of the ring buffer implementation in > > drivers/hid/hid-debug.c. This implementation has certain problem points: > > > > - it may stuck in an infinite loop > > - it may return corrupted data > > - a reader and a writer are not protected by spinlocks, which can lead to > > the corrupted data > > > > The suggested patchset is a new ring buffer implementation which overwrites > > the oldest data in case of an overflow. One can verify the suggested ring > > buffer implementation by fuzzing it with modified kernel and fuzzer-reader > > at: https://gist.github.com/nefigtut/33d56e3870b67493cc867344aed2a062 > > Vladis, > > thanks for cleaning it up. I actually like your rewrite quite a lot. > > Quick question -- how well was it tested in which scenarios? > > -- > Jiri Kosina > SUSE Labs
[PATCH 2/3] HID: debug: provide reader-writer locking for the ring buffer
hdev->debug_list->hid_debug_buf is not protected from concurrent updates
from the writer, hid_debug_event() and reads by the reader,
hid_debug_events_read(). Fix this by adding per-list-element spinlock.
Also introduce a temporary buffer tempbuf so copy_to_user() is not called
from under a spinlock.
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 47 +++
include/linux/hid-debug.h | 1 +
2 files changed, 29 insertions(+), 19 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 20580871b0ec..e827784baf1a 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -663,15 +663,17 @@ void hid_debug_event(struct hid_device *hdev, char *buf)
{
unsigned i;
struct hid_debug_list *list;
- unsigned long flags;
+ unsigned long flags, flags2;
spin_lock_irqsave(>debug_list_lock, flags);
list_for_each_entry(list, >debug_list, node) {
+ spin_lock_irqsave(>list_lock, flags2);
for (i = 0; buf[i]; i++)
list->hid_debug_buf[(list->tail + i) %
HID_DEBUG_BUFSIZE] =
buf[i];
list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
-}
+ spin_unlock_irqrestore(>list_lock, flags2);
+ }
spin_unlock_irqrestore(>debug_list_lock, flags);
wake_up_interruptible(>debug_wait);
@@ -1095,6 +1097,7 @@ static int hid_debug_events_open(struct inode *inode,
struct file *file)
}
list->hdev = (struct hid_device *) inode->i_private;
file->private_data = list;
+ spin_lock_init(>list_lock);
mutex_init(>read_mutex);
spin_lock_irqsave(>hdev->debug_list_lock, flags);
@@ -1109,6 +1112,8 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
size_t count, loff_t *ppos)
{
struct hid_debug_list *list = file->private_data;
+ char *tmpbuf;
+ unsigned long flags;
int ret = 0, len;
DECLARE_WAITQUEUE(wait, current);
@@ -1151,10 +1156,18 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
if (ret)
goto out;
- /* pass the ringbuffer content to userspace */
+ tmpbuf = kmalloc(min_t(size_t, count, HID_DEBUG_BUFSIZE),
+GFP_KERNEL|__GFP_NOWARN);
+ if (!tmpbuf) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ /* lock and copy the ringbuffer content to tmpbuf */
+ spin_lock_irqsave(>list_lock, flags);
copy_rest:
if (list->tail == list->head)
- goto out;
+ goto out_unlock;
/* the data from the head to the tail in the buffer is linear */
if (list->tail > list->head) {
@@ -1162,11 +1174,7 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
if (len > count)
len = count;
- if (copy_to_user(buffer + ret,
- >hid_debug_buf[list->head], len)) {
- ret = -EFAULT;
- goto out;
- }
+ memcpy(tmpbuf + ret, >hid_debug_buf[list->head], len);
ret += len;
list->head += len;
} else {
@@ -1180,19 +1188,11 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
if (len > count) {
len = count;
- if (copy_to_user(buffer,
- >hid_debug_buf[list->head], len)) {
- ret = -EFAULT;
- goto out;
- }
+ memcpy(tmpbuf, >hid_debug_buf[list->head], len);
ret += len;
list->head += len;
} else {
- if (copy_to_user(buffer,
- >hid_debug_buf[list->head], len)) {
- ret = -EFAULT;
- goto out;
- }
+ memcpy(tmpbuf, >hid_debug_buf[list->head], len);
list->head = 0;
ret += len;
count -= len;
@@ -1202,6 +1202,15 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
goto copy_rest;
}
}
+
+out_unlock:
+ spin_unlock_irqrestore(>list_lock, flags);
+
+ /* copy out tmpbuf content to userspace */
+ if (ret && copy_to_user(buffer, tmpbuf, ret))
+ ret = -EFAULT;
+ kfree(tmpbuf);
+
out:
mutex_unlock(>read_mutex);
return ret;
diff --git a/includ
[PATCH 2/3] HID: debug: provide reader-writer locking for the ring buffer
hdev->debug_list->hid_debug_buf is not protected from concurrent updates
from the writer, hid_debug_event() and reads by the reader,
hid_debug_events_read(). Fix this by adding per-list-element spinlock.
Also introduce a temporary buffer tempbuf so copy_to_user() is not called
from under a spinlock.
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 47 +++
include/linux/hid-debug.h | 1 +
2 files changed, 29 insertions(+), 19 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 20580871b0ec..e827784baf1a 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -663,15 +663,17 @@ void hid_debug_event(struct hid_device *hdev, char *buf)
{
unsigned i;
struct hid_debug_list *list;
- unsigned long flags;
+ unsigned long flags, flags2;
spin_lock_irqsave(>debug_list_lock, flags);
list_for_each_entry(list, >debug_list, node) {
+ spin_lock_irqsave(>list_lock, flags2);
for (i = 0; buf[i]; i++)
list->hid_debug_buf[(list->tail + i) %
HID_DEBUG_BUFSIZE] =
buf[i];
list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
-}
+ spin_unlock_irqrestore(>list_lock, flags2);
+ }
spin_unlock_irqrestore(>debug_list_lock, flags);
wake_up_interruptible(>debug_wait);
@@ -1095,6 +1097,7 @@ static int hid_debug_events_open(struct inode *inode,
struct file *file)
}
list->hdev = (struct hid_device *) inode->i_private;
file->private_data = list;
+ spin_lock_init(>list_lock);
mutex_init(>read_mutex);
spin_lock_irqsave(>hdev->debug_list_lock, flags);
@@ -1109,6 +1112,8 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
size_t count, loff_t *ppos)
{
struct hid_debug_list *list = file->private_data;
+ char *tmpbuf;
+ unsigned long flags;
int ret = 0, len;
DECLARE_WAITQUEUE(wait, current);
@@ -1151,10 +1156,18 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
if (ret)
goto out;
- /* pass the ringbuffer content to userspace */
+ tmpbuf = kmalloc(min_t(size_t, count, HID_DEBUG_BUFSIZE),
+GFP_KERNEL|__GFP_NOWARN);
+ if (!tmpbuf) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ /* lock and copy the ringbuffer content to tmpbuf */
+ spin_lock_irqsave(>list_lock, flags);
copy_rest:
if (list->tail == list->head)
- goto out;
+ goto out_unlock;
/* the data from the head to the tail in the buffer is linear */
if (list->tail > list->head) {
@@ -1162,11 +1174,7 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
if (len > count)
len = count;
- if (copy_to_user(buffer + ret,
- >hid_debug_buf[list->head], len)) {
- ret = -EFAULT;
- goto out;
- }
+ memcpy(tmpbuf + ret, >hid_debug_buf[list->head], len);
ret += len;
list->head += len;
} else {
@@ -1180,19 +1188,11 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
if (len > count) {
len = count;
- if (copy_to_user(buffer,
- >hid_debug_buf[list->head], len)) {
- ret = -EFAULT;
- goto out;
- }
+ memcpy(tmpbuf, >hid_debug_buf[list->head], len);
ret += len;
list->head += len;
} else {
- if (copy_to_user(buffer,
- >hid_debug_buf[list->head], len)) {
- ret = -EFAULT;
- goto out;
- }
+ memcpy(tmpbuf, >hid_debug_buf[list->head], len);
list->head = 0;
ret += len;
count -= len;
@@ -1202,6 +1202,15 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
goto copy_rest;
}
}
+
+out_unlock:
+ spin_unlock_irqrestore(>list_lock, flags);
+
+ /* copy out tmpbuf content to userspace */
+ if (ret && copy_to_user(buffer, tmpbuf, ret))
+ ret = -EFAULT;
+ kfree(tmpbuf);
+
out:
mutex_unlock(>read_mutex);
return ret;
diff --git a/includ
[PATCH 3/3] HID: debug: fix the ring buffer writer implementation
ring buffer implementation hid_debug_event() is strange allowing a lost
of data. it does not move head pointer on append and uses per-byte for()
loop. fix this by introducing a new ring buffer implementation, which
overwrites the oldest data in case of the ring buffer overflow. it uses
some calculations for the buffer pointers but only 2 or less memcpy()
calls.
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 66 -
1 file changed, 58 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index de640e4132c7..63247ac4a9ce 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -661,20 +661,70 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
/* enqueue string to 'events' ring buffer */
void hid_debug_event(struct hid_device *hdev, char *buf)
{
- unsigned i;
struct hid_debug_list *list;
- unsigned long flags, flags2;
+ unsigned long flags1, flags2;
+ unsigned int part1, part2;
+ size_t len;
- spin_lock_irqsave(>debug_list_lock, flags);
+ if (!buf || !buf[0])
+ return;
+
+ len = strlen(buf);
+
+ spin_lock_irqsave(>debug_list_lock, flags1);
list_for_each_entry(list, >debug_list, node) {
spin_lock_irqsave(>list_lock, flags2);
- for (i = 0; buf[i]; i++)
- list->hid_debug_buf[(list->tail + i) %
HID_DEBUG_BUFSIZE] =
- buf[i];
- list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
+
+ /* len >= BUFSIZE, copy the last HID_DEBUG_BUFSIZE-1 bytes */
+ if (len > HID_DEBUG_BUFSIZE - 1) {
+ memcpy(list->hid_debug_buf,
+ buf + len - (HID_DEBUG_BUFSIZE - 1),
+ HID_DEBUG_BUFSIZE - 1);
+
+ list->head = 0;
+ list->tail = HID_DEBUG_BUFSIZE - 1;
+ } else {
+ /* append data to the ring buffer */
+ part1 = min_t(size_t, HID_DEBUG_BUFSIZE - list->tail,
+ len);
+ part2 = len - part1;
+
+ memcpy(list->hid_debug_buf + list->tail, buf, part1);
+
+ /* check if head must be moved as the ring is full */
+ if ((list->tail < list->head) &&
+ (list->tail + part1 >= list->head)) {
+ list->head = list->tail + part1 + 1;
+
+ if (list->head >= HID_DEBUG_BUFSIZE)
+ list->head -= HID_DEBUG_BUFSIZE;
+ }
+
+ /* set tail after the end of appended data */
+ list->tail = list->tail + part1;
+ if (list->tail == HID_DEBUG_BUFSIZE) {
+ list->tail = 0;
+ if (list->head == 0)
+ list->head = 1;
+ }
+
+ /* append the 2nd part of data to the ring */
+ if (part2 > 0) {
+ memcpy(list->hid_debug_buf, buf + part1, part2);
+
+ /* check if head must be moved again as
+* the ring is full
+*/
+ if (list->tail + part2 >= list->head)
+ list->head = list->tail + part2 + 1;
+
+ list->tail = list->tail + part2;
+ }
+ }
+
spin_unlock_irqrestore(>list_lock, flags2);
}
- spin_unlock_irqrestore(>debug_list_lock, flags);
+ spin_unlock_irqrestore(>debug_list_lock, flags1);
wake_up_interruptible(>debug_wait);
}
--
2.19.0
[PATCH 3/3] HID: debug: fix the ring buffer writer implementation
ring buffer implementation hid_debug_event() is strange allowing a lost
of data. it does not move head pointer on append and uses per-byte for()
loop. fix this by introducing a new ring buffer implementation, which
overwrites the oldest data in case of the ring buffer overflow. it uses
some calculations for the buffer pointers but only 2 or less memcpy()
calls.
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 66 -
1 file changed, 58 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index de640e4132c7..63247ac4a9ce 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -661,20 +661,70 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
/* enqueue string to 'events' ring buffer */
void hid_debug_event(struct hid_device *hdev, char *buf)
{
- unsigned i;
struct hid_debug_list *list;
- unsigned long flags, flags2;
+ unsigned long flags1, flags2;
+ unsigned int part1, part2;
+ size_t len;
- spin_lock_irqsave(>debug_list_lock, flags);
+ if (!buf || !buf[0])
+ return;
+
+ len = strlen(buf);
+
+ spin_lock_irqsave(>debug_list_lock, flags1);
list_for_each_entry(list, >debug_list, node) {
spin_lock_irqsave(>list_lock, flags2);
- for (i = 0; buf[i]; i++)
- list->hid_debug_buf[(list->tail + i) %
HID_DEBUG_BUFSIZE] =
- buf[i];
- list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
+
+ /* len >= BUFSIZE, copy the last HID_DEBUG_BUFSIZE-1 bytes */
+ if (len > HID_DEBUG_BUFSIZE - 1) {
+ memcpy(list->hid_debug_buf,
+ buf + len - (HID_DEBUG_BUFSIZE - 1),
+ HID_DEBUG_BUFSIZE - 1);
+
+ list->head = 0;
+ list->tail = HID_DEBUG_BUFSIZE - 1;
+ } else {
+ /* append data to the ring buffer */
+ part1 = min_t(size_t, HID_DEBUG_BUFSIZE - list->tail,
+ len);
+ part2 = len - part1;
+
+ memcpy(list->hid_debug_buf + list->tail, buf, part1);
+
+ /* check if head must be moved as the ring is full */
+ if ((list->tail < list->head) &&
+ (list->tail + part1 >= list->head)) {
+ list->head = list->tail + part1 + 1;
+
+ if (list->head >= HID_DEBUG_BUFSIZE)
+ list->head -= HID_DEBUG_BUFSIZE;
+ }
+
+ /* set tail after the end of appended data */
+ list->tail = list->tail + part1;
+ if (list->tail == HID_DEBUG_BUFSIZE) {
+ list->tail = 0;
+ if (list->head == 0)
+ list->head = 1;
+ }
+
+ /* append the 2nd part of data to the ring */
+ if (part2 > 0) {
+ memcpy(list->hid_debug_buf, buf + part1, part2);
+
+ /* check if head must be moved again as
+* the ring is full
+*/
+ if (list->tail + part2 >= list->head)
+ list->head = list->tail + part2 + 1;
+
+ list->tail = list->tail + part2;
+ }
+ }
+
spin_unlock_irqrestore(>list_lock, flags2);
}
- spin_unlock_irqrestore(>debug_list_lock, flags);
+ spin_unlock_irqrestore(>debug_list_lock, flags1);
wake_up_interruptible(>debug_wait);
}
--
2.19.0
[PATCH 0/3] HID: debug: fix the ring buffer implementation
This patchset is fixing some aspects of the ring buffer implementation in drivers/hid/hid-debug.c. This implementation has certain problem points: - it may stuck in an infinite loop - it may return corrupted data - a reader and a writer are not protected by spinlocks, which can lead to the corrupted data The suggested patchset is a new ring buffer implementation which overwrites the oldest data in case of an overflow. One can verify the suggested ring buffer implementation by fuzzing it with modified kernel and fuzzer-reader at: https://gist.github.com/nefigtut/33d56e3870b67493cc867344aed2a062 Vladis Dronov (3): HID: debug: avoid infinite loop and corrupting data HID: debug: provide reader-writer locking for the ring buffer HID: debug: fix ring buffer implementation drivers/hid/hid-debug.c | 201 ++ include/linux/hid-debug.h | 1 + 2 files changed, 142 insertions(+), 60 deletions(-) -- 2.19.0
[PATCH 1/3] HID: debug: avoid infinite loop and corrupting data
hid_debug_events_read() does not properly handle the case when tail < head
and count < HID_DEBUG_BUFSIZE - head and returns corrupted data. Also,
after commit 717adfdaf147 ("HID: debug: check length before
copy_to_user()") it can enter an infinite loop if called with count == 0.
Fix this by properly handling this case and adding a check.
Also, the function has "while (ret == 0)" loop which is not needed, remove
it.
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 109
1 file changed, 65 insertions(+), 44 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index b48100236df8..20580871b0ec 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -722,7 +722,7 @@ void hid_dump_input(struct hid_device *hdev, struct
hid_usage *usage, __s32 valu
hid_debug_event(hdev, buf);
kfree(buf);
-wake_up_interruptible(>debug_wait);
+ wake_up_interruptible(>debug_wait);
}
EXPORT_SYMBOL_GPL(hid_dump_input);
@@ -1112,73 +1112,95 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
int ret = 0, len;
DECLARE_WAITQUEUE(wait, current);
- mutex_lock(>read_mutex);
- while (ret == 0) {
- if (list->head == list->tail) {
- add_wait_queue(>hdev->debug_wait, );
- set_current_state(TASK_INTERRUPTIBLE);
+ if (!count)
+ return 0;
- while (list->head == list->tail) {
- if (file->f_flags & O_NONBLOCK) {
- ret = -EAGAIN;
- break;
- }
- if (signal_pending(current)) {
- ret = -ERESTARTSYS;
- break;
- }
+ mutex_lock(>read_mutex);
+ if (list->head == list->tail) {
+ add_wait_queue(>hdev->debug_wait, );
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ while (list->head == list->tail) {
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ break;
+ }
- if (!list->hdev || !list->hdev->debug) {
- ret = -EIO;
- set_current_state(TASK_RUNNING);
- goto out;
- }
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
- /* allow O_NONBLOCK from other threads */
- mutex_unlock(>read_mutex);
- schedule();
- mutex_lock(>read_mutex);
- set_current_state(TASK_INTERRUPTIBLE);
+ if (!list->hdev || !list->hdev->debug) {
+ ret = -EIO;
+ set_current_state(TASK_RUNNING);
+ goto out;
}
- set_current_state(TASK_RUNNING);
- remove_wait_queue(>hdev->debug_wait, );
+ /* allow O_NONBLOCK from other threads */
+ mutex_unlock(>read_mutex);
+ schedule();
+ mutex_lock(>read_mutex);
+ set_current_state(TASK_INTERRUPTIBLE);
}
- if (ret)
- goto out;
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(>hdev->debug_wait, );
+ }
+
+ if (ret)
+ goto out;
- /* pass the ringbuffer contents to userspace */
+ /* pass the ringbuffer content to userspace */
copy_rest:
- if (list->tail == list->head)
- goto out;
- if (list->tail > list->head) {
- len = list->tail - list->head;
- if (len > count)
- len = count;
+ if (list->tail == list->head)
+ goto out;
+
+ /* the data from the head to the tail in the buffer is linear */
+ if (list->tail > list->head) {
+ len = list->tail - list->head;
+ if (len > count)
+ len = count;
- if (copy_to_user(buffer + ret,
>hid_debug_buf[list->head], len)) {
+ if (copy_to_user(buffe
[PATCH 0/3] HID: debug: fix the ring buffer implementation
This patchset is fixing some aspects of the ring buffer implementation in drivers/hid/hid-debug.c. This implementation has certain problem points: - it may stuck in an infinite loop - it may return corrupted data - a reader and a writer are not protected by spinlocks, which can lead to the corrupted data The suggested patchset is a new ring buffer implementation which overwrites the oldest data in case of an overflow. One can verify the suggested ring buffer implementation by fuzzing it with modified kernel and fuzzer-reader at: https://gist.github.com/nefigtut/33d56e3870b67493cc867344aed2a062 Vladis Dronov (3): HID: debug: avoid infinite loop and corrupting data HID: debug: provide reader-writer locking for the ring buffer HID: debug: fix ring buffer implementation drivers/hid/hid-debug.c | 201 ++ include/linux/hid-debug.h | 1 + 2 files changed, 142 insertions(+), 60 deletions(-) -- 2.19.0
[PATCH 1/3] HID: debug: avoid infinite loop and corrupting data
hid_debug_events_read() does not properly handle the case when tail < head
and count < HID_DEBUG_BUFSIZE - head and returns corrupted data. Also,
after commit 717adfdaf147 ("HID: debug: check length before
copy_to_user()") it can enter an infinite loop if called with count == 0.
Fix this by properly handling this case and adding a check.
Also, the function has "while (ret == 0)" loop which is not needed, remove
it.
Signed-off-by: Vladis Dronov
---
drivers/hid/hid-debug.c | 109
1 file changed, 65 insertions(+), 44 deletions(-)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index b48100236df8..20580871b0ec 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -722,7 +722,7 @@ void hid_dump_input(struct hid_device *hdev, struct
hid_usage *usage, __s32 valu
hid_debug_event(hdev, buf);
kfree(buf);
-wake_up_interruptible(>debug_wait);
+ wake_up_interruptible(>debug_wait);
}
EXPORT_SYMBOL_GPL(hid_dump_input);
@@ -1112,73 +1112,95 @@ static ssize_t hid_debug_events_read(struct file *file,
char __user *buffer,
int ret = 0, len;
DECLARE_WAITQUEUE(wait, current);
- mutex_lock(>read_mutex);
- while (ret == 0) {
- if (list->head == list->tail) {
- add_wait_queue(>hdev->debug_wait, );
- set_current_state(TASK_INTERRUPTIBLE);
+ if (!count)
+ return 0;
- while (list->head == list->tail) {
- if (file->f_flags & O_NONBLOCK) {
- ret = -EAGAIN;
- break;
- }
- if (signal_pending(current)) {
- ret = -ERESTARTSYS;
- break;
- }
+ mutex_lock(>read_mutex);
+ if (list->head == list->tail) {
+ add_wait_queue(>hdev->debug_wait, );
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ while (list->head == list->tail) {
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ break;
+ }
- if (!list->hdev || !list->hdev->debug) {
- ret = -EIO;
- set_current_state(TASK_RUNNING);
- goto out;
- }
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
- /* allow O_NONBLOCK from other threads */
- mutex_unlock(>read_mutex);
- schedule();
- mutex_lock(>read_mutex);
- set_current_state(TASK_INTERRUPTIBLE);
+ if (!list->hdev || !list->hdev->debug) {
+ ret = -EIO;
+ set_current_state(TASK_RUNNING);
+ goto out;
}
- set_current_state(TASK_RUNNING);
- remove_wait_queue(>hdev->debug_wait, );
+ /* allow O_NONBLOCK from other threads */
+ mutex_unlock(>read_mutex);
+ schedule();
+ mutex_lock(>read_mutex);
+ set_current_state(TASK_INTERRUPTIBLE);
}
- if (ret)
- goto out;
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(>hdev->debug_wait, );
+ }
+
+ if (ret)
+ goto out;
- /* pass the ringbuffer contents to userspace */
+ /* pass the ringbuffer content to userspace */
copy_rest:
- if (list->tail == list->head)
- goto out;
- if (list->tail > list->head) {
- len = list->tail - list->head;
- if (len > count)
- len = count;
+ if (list->tail == list->head)
+ goto out;
+
+ /* the data from the head to the tail in the buffer is linear */
+ if (list->tail > list->head) {
+ len = list->tail - list->head;
+ if (len > count)
+ len = count;
- if (copy_to_user(buffer + ret,
>hid_debug_buf[list->head], len)) {
+ if (copy_to_user(buffe
Re: KMSAN: uninit-value in __dev_mc_add
Hello, Eric, all,
> I dunno, your patch looks quite not the right fix.
I agree, it looks more like a dirty hack. Unfortunately, I lack the deep
expertise in the network stack subsystem, so I've posted the patch to,
sort of, start a discussion and probably get some hints.
> If TUN is able to change dev->type, how comes it does not set the
> appropriate dev->addr_len at the same time ?
Well,... probably, nobody cared to do so:
[drivers/net/tun.c]
case TUNSETLINK:
...
tun->dev->type = (int) arg; //<--- that's all!
tun_debug(KERN_INFO, tun, "linktype set to %d\n",
tun->dev->type);
ret = 0;
}
break;
> Really the bug seems to be deeper, and without setting proper
> dev->addr_len, we'll need more 'fixes' like yours.
Absolutely. Unfortunately, I wasn't able to just write such deeper patch.
Let me share what I have found and let me hope to get an advise.
- So setting just the tun->dev->type makes the dev struct inconsistent.
- There are more field to adjust, at least dev->broadcast. Also, there are
a number of *_ops fields which are all set for the Ethernet type, most
probably they must be adjusted also.
- There is no get_addr_len_by_link_type() or a simple way to get link layer
properties by dev->type. Such settings are scattered in *_setup and
*_init functions, like ipgre_tunnel_init() { ... dev->addr_len = 4; ...}
Having these, I can imagine 2 ways for a proper fix.
1) Destroy the net_device in question and recreate it when changing a link
type. This way all the dev fields are set right. Create it in a similar way
as rtnl_newlink() does. Again, we do not have get_X_by_link_type(), so it
probably will be some large switch()/case:
$ grep '^#define ARPHRD_' include/uapi/linux/if_arp.h | wc -l
59
2) Leave tun an Ethernet device, add some tun->pretend_to_be_this_link_type
field and change only it on TUNSETLINK. And use this field in cases for which
TUNSETLINK was invented in the first place. Unfortunately, I do not have such
a list. The initial the commit ff4cc3ac93e1 says:
For use with
wireless and other networking types it should be possible to set the
ARP type via an ioctl.
Surely, there can be something else which I do not see. Could anyone suggest
an advice on this?
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
Re: KMSAN: uninit-value in __dev_mc_add
Hello, Eric, all,
> I dunno, your patch looks quite not the right fix.
I agree, it looks more like a dirty hack. Unfortunately, I lack the deep
expertise in the network stack subsystem, so I've posted the patch to,
sort of, start a discussion and probably get some hints.
> If TUN is able to change dev->type, how comes it does not set the
> appropriate dev->addr_len at the same time ?
Well,... probably, nobody cared to do so:
[drivers/net/tun.c]
case TUNSETLINK:
...
tun->dev->type = (int) arg; //<--- that's all!
tun_debug(KERN_INFO, tun, "linktype set to %d\n",
tun->dev->type);
ret = 0;
}
break;
> Really the bug seems to be deeper, and without setting proper
> dev->addr_len, we'll need more 'fixes' like yours.
Absolutely. Unfortunately, I wasn't able to just write such deeper patch.
Let me share what I have found and let me hope to get an advise.
- So setting just the tun->dev->type makes the dev struct inconsistent.
- There are more field to adjust, at least dev->broadcast. Also, there are
a number of *_ops fields which are all set for the Ethernet type, most
probably they must be adjusted also.
- There is no get_addr_len_by_link_type() or a simple way to get link layer
properties by dev->type. Such settings are scattered in *_setup and
*_init functions, like ipgre_tunnel_init() { ... dev->addr_len = 4; ...}
Having these, I can imagine 2 ways for a proper fix.
1) Destroy the net_device in question and recreate it when changing a link
type. This way all the dev fields are set right. Create it in a similar way
as rtnl_newlink() does. Again, we do not have get_X_by_link_type(), so it
probably will be some large switch()/case:
$ grep '^#define ARPHRD_' include/uapi/linux/if_arp.h | wc -l
59
2) Leave tun an Ethernet device, add some tun->pretend_to_be_this_link_type
field and change only it on TUNSETLINK. And use this field in cases for which
TUNSETLINK was invented in the first place. Unfortunately, I do not have such
a list. The initial the commit ff4cc3ac93e1 says:
For use with
wireless and other networking types it should be possible to set the
ARP type via an ioctl.
Surely, there can be something else which I do not see. Could anyone suggest
an advice on this?
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
Re: KMSAN: uninit-value in __dev_mc_add
Hello,
This report is actually for the same bug which was reported in:
https://syzkaller.appspot.com/bug?id=088efeac32fdde781038a777a63e436c0d4d7036
The note there that the bug was fixed by "Commits: net: fix uninit-value in
__hw_addr_add_ex()" is wrong. A C-reproducer from the 2nd syzkaller report
can trigger the bug from this one.
I've researched this and a result is a proposed patch, the problem is the tun
device code allowing to set an arbitrary link type.
https://lkml.org/lkml/2018/9/26/416
https://lore.kernel.org/lkml/20180926093018.6646-1-vdro...@redhat.com/T/#u
https://marc.info/?l=linux-netdev=153795423320016=2
A simplified reproducer is attached.
Best regards,
Vladis Dronov
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int main(int argc, char **argv)
{
int ret, sockfd, tunfd;
syscall(__NR_mmap, 0x2000, 0x100, 3, 0x32, -1, 0);
// socket(AF_PACKET, SOCK_DGRAM|SOCK_NONBLOCK, 0)
sockfd = syscall(__NR_socket, 0x11, 0x10802, 0);
if (sockfd < 0) {
perror("socket()");
ret = 1;
goto exit_end;
}
memcpy((void*)0x2240, "/dev/net/tun", 13);
tunfd = open((char *)0x2240, 0);
if (tunfd < 0) {
perror("open()");
ret = 2;
goto exit_sock_close;
}
memcpy((void*)0x20c0,
"\x69\x67\x62\x30\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16);
*(uint16_t*)0x20d0 = 0x4012;
ret = syscall(__NR_ioctl, tunfd, 0x400454ca, 0x20c0); // TUNSETIFF
_IOW('T', 202, int)
if (ret < 0) {
perror("ioctl(TUNSETIFF)");
ret = 3;
goto exit_tun_close;
}
// TUNSETLINK _IOW('T', 205, int) / 0x30a = 778 = ARPHRD_IPGRE
if (argc < 2)
ret = syscall(__NR_ioctl, tunfd, 0x400454cd, 0x30a);
else
ret = syscall(__NR_ioctl, tunfd, 0x400454cd, atoi(argv[1]));
if (ret < 0) {
perror("ioctl(TUNSETLINK)");
ret = 4;
goto exit_tun_close;
}
memcpy((void*)0x2040,
"\x69\x67\x62\x30\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16);
*(uint16_t*)0x2050 = 0xa201;
ret = syscall(__NR_ioctl, sockfd, 0x8914, 0x2040); // SIOCSIFFLAGS 0x8914
if (ret < 0) {
perror("ioctl(SIOCSIFFLAGS)");
ret = 5;
goto exit_tun_close;
}
printf("done:\n");
system("/usr/sbin/ip -details link show igb0");
exit_tun_close:
close(tunfd);
exit_sock_close:
close(sockfd);
exit_end:
munmap((void *)0x2000, 0x100);
return 0;
}
Re: KMSAN: uninit-value in __dev_mc_add
Hello,
This report is actually for the same bug which was reported in:
https://syzkaller.appspot.com/bug?id=088efeac32fdde781038a777a63e436c0d4d7036
The note there that the bug was fixed by "Commits: net: fix uninit-value in
__hw_addr_add_ex()" is wrong. A C-reproducer from the 2nd syzkaller report
can trigger the bug from this one.
I've researched this and a result is a proposed patch, the problem is the tun
device code allowing to set an arbitrary link type.
https://lkml.org/lkml/2018/9/26/416
https://lore.kernel.org/lkml/20180926093018.6646-1-vdro...@redhat.com/T/#u
https://marc.info/?l=linux-netdev=153795423320016=2
A simplified reproducer is attached.
Best regards,
Vladis Dronov
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int main(int argc, char **argv)
{
int ret, sockfd, tunfd;
syscall(__NR_mmap, 0x2000, 0x100, 3, 0x32, -1, 0);
// socket(AF_PACKET, SOCK_DGRAM|SOCK_NONBLOCK, 0)
sockfd = syscall(__NR_socket, 0x11, 0x10802, 0);
if (sockfd < 0) {
perror("socket()");
ret = 1;
goto exit_end;
}
memcpy((void*)0x2240, "/dev/net/tun", 13);
tunfd = open((char *)0x2240, 0);
if (tunfd < 0) {
perror("open()");
ret = 2;
goto exit_sock_close;
}
memcpy((void*)0x20c0,
"\x69\x67\x62\x30\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16);
*(uint16_t*)0x20d0 = 0x4012;
ret = syscall(__NR_ioctl, tunfd, 0x400454ca, 0x20c0); // TUNSETIFF
_IOW('T', 202, int)
if (ret < 0) {
perror("ioctl(TUNSETIFF)");
ret = 3;
goto exit_tun_close;
}
// TUNSETLINK _IOW('T', 205, int) / 0x30a = 778 = ARPHRD_IPGRE
if (argc < 2)
ret = syscall(__NR_ioctl, tunfd, 0x400454cd, 0x30a);
else
ret = syscall(__NR_ioctl, tunfd, 0x400454cd, atoi(argv[1]));
if (ret < 0) {
perror("ioctl(TUNSETLINK)");
ret = 4;
goto exit_tun_close;
}
memcpy((void*)0x2040,
"\x69\x67\x62\x30\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16);
*(uint16_t*)0x2050 = 0xa201;
ret = syscall(__NR_ioctl, sockfd, 0x8914, 0x2040); // SIOCSIFFLAGS 0x8914
if (ret < 0) {
perror("ioctl(SIOCSIFFLAGS)");
ret = 5;
goto exit_tun_close;
}
printf("done:\n");
system("/usr/sbin/ip -details link show igb0");
exit_tun_close:
close(tunfd);
exit_sock_close:
close(sockfd);
exit_end:
munmap((void *)0x2000, 0x100);
return 0;
}
[PATCH] nl80211: check for the required netlink attributes presence
nl80211_set_rekey_data() does not check if the required attributes
NL80211_REKEY_DATA_{REPLAY_CTR,KEK,KCK} are present when processing
NL80211_CMD_SET_REKEY_OFFLOAD request. This request can be issued by
users with CAP_NET_ADMIN privilege and may result in NULL dereference
and a system crash. Add a check for the required attributes presence.
This patch is based on the patch by bo Zhang.
This fixes CVE-2017-12153.
References: https://bugzilla.redhat.com/show_bug.cgi?id=1491046
Fixes: e5497d766ad ("cfg80211/nl80211: support GTK rekey offload")
Cc: <sta...@vger.kernel.org> # v3.1-rc1
Reported-by: bo Zhang <zhangbo5891...@gmail.com>
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
net/wireless/nl80211.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 0df8023..fbd5593 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10903,6 +10903,9 @@ static int nl80211_set_rekey_data(struct sk_buff *skb,
struct genl_info *info)
if (err)
return err;
+ if (!tb[NL80211_REKEY_DATA_REPLAY_CTR] || !tb[NL80211_REKEY_DATA_KEK] ||
+ !tb[NL80211_REKEY_DATA_KCK])
+ return -EINVAL;
if (nla_len(tb[NL80211_REKEY_DATA_REPLAY_CTR]) !=
NL80211_REPLAY_CTR_LEN)
return -ERANGE;
if (nla_len(tb[NL80211_REKEY_DATA_KEK]) != NL80211_KEK_LEN)
--
2.9.5
[PATCH] nl80211: check for the required netlink attributes presence
nl80211_set_rekey_data() does not check if the required attributes
NL80211_REKEY_DATA_{REPLAY_CTR,KEK,KCK} are present when processing
NL80211_CMD_SET_REKEY_OFFLOAD request. This request can be issued by
users with CAP_NET_ADMIN privilege and may result in NULL dereference
and a system crash. Add a check for the required attributes presence.
This patch is based on the patch by bo Zhang.
This fixes CVE-2017-12153.
References: https://bugzilla.redhat.com/show_bug.cgi?id=1491046
Fixes: e5497d766ad ("cfg80211/nl80211: support GTK rekey offload")
Cc: # v3.1-rc1
Reported-by: bo Zhang
Signed-off-by: Vladis Dronov
---
net/wireless/nl80211.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 0df8023..fbd5593 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10903,6 +10903,9 @@ static int nl80211_set_rekey_data(struct sk_buff *skb,
struct genl_info *info)
if (err)
return err;
+ if (!tb[NL80211_REKEY_DATA_REPLAY_CTR] || !tb[NL80211_REKEY_DATA_KEK] ||
+ !tb[NL80211_REKEY_DATA_KCK])
+ return -EINVAL;
if (nla_len(tb[NL80211_REKEY_DATA_REPLAY_CTR]) !=
NL80211_REPLAY_CTR_LEN)
return -ERANGE;
if (nla_len(tb[NL80211_REKEY_DATA_KEK]) != NL80211_KEK_LEN)
--
2.9.5
[PATCH] video: fbdev: aty: do not leak uninitialized padding in clk to userspace
'clk' is copied to a userland with padding byte(s) after 'vclk_post_div'
field unitialized, leaking data from the stack. Fix this ensuring all of
'clk' is initialized to zero.
References: https://github.com/torvalds/linux/pull/441
Reported-by: sohu0106 <sohu0...@126.com>
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
drivers/video/fbdev/aty/atyfb_base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/aty/atyfb_base.c
b/drivers/video/fbdev/aty/atyfb_base.c
index b55fdac..e4c91d7 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -1855,7 +1855,7 @@ static int atyfb_ioctl(struct fb_info *info, u_int cmd,
u_long arg)
#if defined(DEBUG) && defined(CONFIG_FB_ATY_CT)
case ATYIO_CLKR:
if (M64_HAS(INTEGRATED)) {
- struct atyclk clk;
+ struct atyclk clk = { 0 };
union aty_pll *pll = >pll;
u32 dsp_config = pll->ct.dsp_config;
u32 dsp_on_off = pll->ct.dsp_on_off;
--
2.9.5
[PATCH] video: fbdev: aty: do not leak uninitialized padding in clk to userspace
'clk' is copied to a userland with padding byte(s) after 'vclk_post_div'
field unitialized, leaking data from the stack. Fix this ensuring all of
'clk' is initialized to zero.
References: https://github.com/torvalds/linux/pull/441
Reported-by: sohu0106
Signed-off-by: Vladis Dronov
---
drivers/video/fbdev/aty/atyfb_base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/aty/atyfb_base.c
b/drivers/video/fbdev/aty/atyfb_base.c
index b55fdac..e4c91d7 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -1855,7 +1855,7 @@ static int atyfb_ioctl(struct fb_info *info, u_int cmd,
u_long arg)
#if defined(DEBUG) && defined(CONFIG_FB_ATY_CT)
case ATYIO_CLKR:
if (M64_HAS(INTEGRATED)) {
- struct atyclk clk;
+ struct atyclk clk = { 0 };
union aty_pll *pll = >pll;
u32 dsp_config = pll->ct.dsp_config;
u32 dsp_on_off = pll->ct.dsp_on_off;
--
2.9.5
[PATCH] xfrm: policy: check policy direction value
The 'dir' parameter in xfrm_migrate() is a user-controlled byte which is used
as an array index. This can lead to an out-of-bound access, kernel lockup and
DoS. Add a check for the 'dir' value.
This fixes CVE-2017-11600.
References: https://bugzilla.redhat.com/show_bug.cgi?id=1474928
Fixes: 80c9abaabf42 ("[XFRM]: Extension for dynamic update of endpoint
address(es)")
Cc: <sta...@vger.kernel.org> # v2.6.21-rc1
Reported-by: "bo Zhang" <zhangbo5891...@gmail.com>
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
net/xfrm/xfrm_policy.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ff61d85..6f5a0dad 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3308,9 +3308,15 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8
dir, u8 type,
struct xfrm_state *x_new[XFRM_MAX_DEPTH];
struct xfrm_migrate *mp;
+ /* Stage 0 - sanity checks */
if ((err = xfrm_migrate_check(m, num_migrate)) < 0)
goto out;
+ if (dir >= XFRM_POLICY_MAX) {
+ err = -EINVAL;
+ goto out;
+ }
+
/* Stage 1 - find policy */
if ((pol = xfrm_migrate_policy_find(sel, dir, type, net)) == NULL) {
err = -ENOENT;
--
2.9.4
[PATCH] xfrm: policy: check policy direction value
The 'dir' parameter in xfrm_migrate() is a user-controlled byte which is used
as an array index. This can lead to an out-of-bound access, kernel lockup and
DoS. Add a check for the 'dir' value.
This fixes CVE-2017-11600.
References: https://bugzilla.redhat.com/show_bug.cgi?id=1474928
Fixes: 80c9abaabf42 ("[XFRM]: Extension for dynamic update of endpoint
address(es)")
Cc: # v2.6.21-rc1
Reported-by: "bo Zhang"
Signed-off-by: Vladis Dronov
---
net/xfrm/xfrm_policy.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ff61d85..6f5a0dad 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3308,9 +3308,15 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8
dir, u8 type,
struct xfrm_state *x_new[XFRM_MAX_DEPTH];
struct xfrm_migrate *mp;
+ /* Stage 0 - sanity checks */
if ((err = xfrm_migrate_check(m, num_migrate)) < 0)
goto out;
+ if (dir >= XFRM_POLICY_MAX) {
+ err = -EINVAL;
+ goto out;
+ }
+
/* Stage 1 - find policy */
if ((pol = xfrm_migrate_policy_find(sel, dir, type, net)) == NULL) {
err = -ENOENT;
--
2.9.4
[PATCH v2] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is a user-controlled 'uint32_t' value which is used as a loop count limit. This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. References: https://bugzilla.redhat.com/show_bug.cgi?id=1437431 Signed-off-by: Vladis Dronov <vdro...@redhat.com> --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index b445ce9..e0d7ff9 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -1281,6 +1281,9 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, if (req->multisample_count != 0) return -EINVAL; + if (req->mip_levels > DRM_VMW_MAX_MIP_LEVELS) + return -EINVAL; + if (unlikely(vmw_user_surface_size == 0)) vmw_user_surface_size = ttm_round_pot(sizeof(*user_srf)) + 128; -- 2.9.3
[PATCH v2] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is a user-controlled 'uint32_t' value which is used as a loop count limit. This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. References: https://bugzilla.redhat.com/show_bug.cgi?id=1437431 Signed-off-by: Vladis Dronov --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index b445ce9..e0d7ff9 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -1281,6 +1281,9 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, if (req->multisample_count != 0) return -EINVAL; + if (req->mip_levels > DRM_VMW_MAX_MIP_LEVELS) + return -EINVAL; + if (unlikely(vmw_user_surface_size == 0)) vmw_user_surface_size = ttm_round_pot(sizeof(*user_srf)) + 128; -- 2.9.3
Re: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
Hello, Sinclair! > Here, the check should be "> DRM_VMW_MAX_MIP_LEVELS" because req->mip_levels > is only for one layer. Got it, thanks! > Also, as long as we can doing a check, I would suggest we check for 0 as > well. Do you mean a check for "req->mip_levels > 0" or for "req->mip_levels >= 0" ? I glimpsed thru the code and I do not see problems with req->mip_levels being 0, surely I may be mistaking. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer - Original Message - From: "Sinclair Yeh" <s...@vmware.com> To: "Vladis Dronov" <vdro...@redhat.com> Cc: "VMware Graphics" <linux-graphics-maintai...@vmware.com>, "Thomas Hellstrom" <thellst...@vmware.com>, "David Airlie" <airl...@linux.ie>, dri-de...@lists.freedesktop.org, linux-kernel@vger.kernel.org Sent: Friday, March 31, 2017 5:07:12 PM Subject: Re: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl() Hi Vladis, On Thu, Mar 30, 2017 at 12:27:12PM +0200, Vladis Dronov wrote: > The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is > a user-controlled 'uint32_t' value which is used as a loop count limit. > This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. > > References: > https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1437431=DwIBAg=uilaK90D4TOVoH58JNXRgQ=HaJ2a6NYExoV0cntAYcoqA=5yR87BuuU86aoAjCveInxh6jvgOyumqIHQhTs0xLo38=tWQs7vwNLgD_b2RWMddVtusEKh9FQTIF5rKFOWudslE= > > Signed-off-by: Vladis Dronov <vdro...@redhat.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index b445ce9..b30824b 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -1281,6 +1281,10 @@ int vmw_gb_surface_define_ioctl(struct drm_device > *dev, void *data, > if (req->multisample_count != 0) > return -EINVAL; > > + if (req->mip_levels > DRM_VMW_MAX_SURFACE_FACES * > + DRM_VMW_MAX_MIP_LEVELS) > + return -EINVAL; > + Here, the check should be "> DRM_VMW_MAX_MIP_LEVELS" because req->mip_levels is only for one layer. Also, as long as we can doing a check, I would suggest we check for 0 as well. Sinclair
Re: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
Hello, Sinclair! > Here, the check should be "> DRM_VMW_MAX_MIP_LEVELS" because req->mip_levels > is only for one layer. Got it, thanks! > Also, as long as we can doing a check, I would suggest we check for 0 as > well. Do you mean a check for "req->mip_levels > 0" or for "req->mip_levels >= 0" ? I glimpsed thru the code and I do not see problems with req->mip_levels being 0, surely I may be mistaking. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer - Original Message - From: "Sinclair Yeh" To: "Vladis Dronov" Cc: "VMware Graphics" , "Thomas Hellstrom" , "David Airlie" , dri-de...@lists.freedesktop.org, linux-kernel@vger.kernel.org Sent: Friday, March 31, 2017 5:07:12 PM Subject: Re: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl() Hi Vladis, On Thu, Mar 30, 2017 at 12:27:12PM +0200, Vladis Dronov wrote: > The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is > a user-controlled 'uint32_t' value which is used as a loop count limit. > This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. > > References: > https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1437431=DwIBAg=uilaK90D4TOVoH58JNXRgQ=HaJ2a6NYExoV0cntAYcoqA=5yR87BuuU86aoAjCveInxh6jvgOyumqIHQhTs0xLo38=tWQs7vwNLgD_b2RWMddVtusEKh9FQTIF5rKFOWudslE= > > Signed-off-by: Vladis Dronov > --- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index b445ce9..b30824b 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -1281,6 +1281,10 @@ int vmw_gb_surface_define_ioctl(struct drm_device > *dev, void *data, > if (req->multisample_count != 0) > return -EINVAL; > > + if (req->mip_levels > DRM_VMW_MAX_SURFACE_FACES * > + DRM_VMW_MAX_MIP_LEVELS) > + return -EINVAL; > + Here, the check should be "> DRM_VMW_MAX_MIP_LEVELS" because req->mip_levels is only for one layer. Also, as long as we can doing a check, I would suggest we check for 0 as well. Sinclair
Re: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
This flaw was assigned an id CVE-2017-7346 by MITRE: http://seclists.org/oss-sec/2017/q1/696 Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer - Original Message - From: "Vladis Dronov" <vdro...@redhat.com> To: "VMware Graphics" <linux-graphics-maintai...@vmware.com>, "Sinclair Yeh" <s...@vmware.com>, "Thomas Hellstrom" <thellst...@vmware.com>, "David Airlie" <airl...@linux.ie>, dri-de...@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: "Vladis Dronov" <vdro...@redhat.com> Sent: Thursday, March 30, 2017 12:27:12 PM Subject: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl() The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is a user-controlled 'uint32_t' value which is used as a loop count limit. This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. References: https://bugzilla.redhat.com/show_bug.cgi?id=1437431 Signed-off-by: Vladis Dronov <vdro...@redhat.com>
Re: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
This flaw was assigned an id CVE-2017-7346 by MITRE: http://seclists.org/oss-sec/2017/q1/696 Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer - Original Message - From: "Vladis Dronov" To: "VMware Graphics" , "Sinclair Yeh" , "Thomas Hellstrom" , "David Airlie" , dri-de...@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: "Vladis Dronov" Sent: Thursday, March 30, 2017 12:27:12 PM Subject: [PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl() The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is a user-controlled 'uint32_t' value which is used as a loop count limit. This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. References: https://bugzilla.redhat.com/show_bug.cgi?id=1437431 Signed-off-by: Vladis Dronov
[PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is a user-controlled 'uint32_t' value which is used as a loop count limit. This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. References: https://bugzilla.redhat.com/show_bug.cgi?id=1437431 Signed-off-by: Vladis Dronov <vdro...@redhat.com> --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index b445ce9..b30824b 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -1281,6 +1281,10 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, if (req->multisample_count != 0) return -EINVAL; + if (req->mip_levels > DRM_VMW_MAX_SURFACE_FACES * + DRM_VMW_MAX_MIP_LEVELS) + return -EINVAL; + if (unlikely(vmw_user_surface_size == 0)) vmw_user_surface_size = ttm_round_pot(sizeof(*user_srf)) + 128; -- 2.9.3
[PATCH] kernel: drm/vmwgfx: limit the number of mip levels in vmw_gb_surface_define_ioctl()
The 'req->mip_levels' parameter in vmw_gb_surface_define_ioctl() is a user-controlled 'uint32_t' value which is used as a loop count limit. This can lead to a kernel lockup and DoS. Add check for 'req->mip_levels'. References: https://bugzilla.redhat.com/show_bug.cgi?id=1437431 Signed-off-by: Vladis Dronov --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index b445ce9..b30824b 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -1281,6 +1281,10 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, if (req->multisample_count != 0) return -EINVAL; + if (req->mip_levels > DRM_VMW_MAX_SURFACE_FACES * + DRM_VMW_MAX_MIP_LEVELS) + return -EINVAL; + if (unlikely(vmw_user_surface_size == 0)) vmw_user_surface_size = ttm_round_pot(sizeof(*user_srf)) + 128; -- 2.9.3
[PATCH] drm/vmwgfx: Check check that number of mip levels is above zero in vmw_surface_define_ioctl()
In vmw_surface_define_ioctl(), a num_sizes parameter is assigned a user-controlled value which is not checked for zero. It is used in a call to kmalloc() which returns ZERO_SIZE_PTR. Later ZERO_SIZE_PTR is dereferenced which leads to a GPF and possibly to a kernel panic. Add the check for zero to avoid this. Reference: https://bugzilla.redhat.com/show_bug.cgi?id=1435719 Signed-off-by: Vladis Dronov <vdro...@redhat.com> --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index b445ce9..42840cc 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -716,8 +716,8 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, for (i = 0; i < DRM_VMW_MAX_SURFACE_FACES; ++i) num_sizes += req->mip_levels[i]; - if (num_sizes > DRM_VMW_MAX_SURFACE_FACES * - DRM_VMW_MAX_MIP_LEVELS) + if (num_sizes <= 0 || + num_sizes > DRM_VMW_MAX_SURFACE_FACES * DRM_VMW_MAX_MIP_LEVELS) return -EINVAL; size = vmw_user_surface_size + 128 + -- 2.9.3
[PATCH] drm/vmwgfx: Check check that number of mip levels is above zero in vmw_surface_define_ioctl()
In vmw_surface_define_ioctl(), a num_sizes parameter is assigned a user-controlled value which is not checked for zero. It is used in a call to kmalloc() which returns ZERO_SIZE_PTR. Later ZERO_SIZE_PTR is dereferenced which leads to a GPF and possibly to a kernel panic. Add the check for zero to avoid this. Reference: https://bugzilla.redhat.com/show_bug.cgi?id=1435719 Signed-off-by: Vladis Dronov --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index b445ce9..42840cc 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -716,8 +716,8 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, for (i = 0; i < DRM_VMW_MAX_SURFACE_FACES; ++i) num_sizes += req->mip_levels[i]; - if (num_sizes > DRM_VMW_MAX_SURFACE_FACES * - DRM_VMW_MAX_MIP_LEVELS) + if (num_sizes <= 0 || + num_sizes > DRM_VMW_MAX_SURFACE_FACES * DRM_VMW_MAX_MIP_LEVELS) return -EINVAL; size = vmw_user_surface_size + 128 + -- 2.9.3
Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
From: Vladis Dronov <vdro...@redhat.com>
Subject: [PATCH] ALSA: usb-audio: Fix double-free in error paths after
snd_usb_add_audio_stream() call
create_fixed_stream_quirk(), snd_usb_parse_audio_interface() and
create_uaxx_quirk() functions allocate the audioformat object by themselves
and free it upon error before returning. However, once the object is linked
to a stream, it's freed again in snd_usb_audio_pcm_free(), thus it'll be
double-freed, eventually resulting in a memory corruption.
This patch fixes these failures in the error paths by unlinking the audioformat
object before freeing it.
Based on a patch by Takashi Iwai" <ti...@suse.de>
[Note for stable backports:
this patch requires the commit 902eb7fd1e4a ('ALSA: usb-audio: Minor
code cleanup in create_fixed_stream_quirk()')]
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
Reported-by: Ralf Spenneberg <r...@spenneberg.net>
Cc: <sta...@vger.kernel.org> # see the note above
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
sound/usb/quirks.c | 4
sound/usb/stream.c | 6 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..6178bb5 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(>list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -193,6 +194,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
return 0;
error:
+ list_del(>list); /* unlink for avoiding double-free */
kfree(fp);
kfree(rate_table);
return err;
@@ -469,6 +471,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
fp->ep_attr = get_endpoint(alts, 0)->bmAttributes;
fp->datainterval = 0;
fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+ INIT_LIST_HEAD(>list);
switch (fp->maxpacksize) {
case 0x120:
@@ -492,6 +495,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(>list); /* unlink for avoiding double-free */
kfree(fp);
return err;
}
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 51258a1..6fe7f21 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -316,7 +316,9 @@ static struct snd_pcm_chmap_elem *convert_chmap(int
channels, unsigned int bits,
/*
* add this endpoint to the chip instance.
* if a stream with the same endpoint already exists, append to it.
- * if not, create a new pcm stream.
+ * if not, create a new pcm stream. note, fp is added to the substream
+ * fmt_list and will be freed on the chip instance release. do not free
+ * fp or do remove it from the substream fmt_list to avoid double-free.
*/
int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
int stream,
@@ -677,6 +679,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio
*chip, int iface_no)
* (fp->maxpacksize & 0x7ff);
fp->attributes = parse_uac_endpoint_attributes(chip, alts,
protocol, iface_no);
fp->clock = clock;
+ INIT_LIST_HEAD(>list);
/* some quirks for attributes here */
@@ -725,6 +728,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio
*chip, int iface_no)
dev_dbg(>dev, "%u:%d: add audio endpoint %#x\n", iface_no,
altno, fp->endpoint);
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(>list); /* unlink for avoiding double-free
*/
kfree(fp->rate_table);
kfree(fp->chmap);
kfree(fp);
--
2.5.5
Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
From: Vladis Dronov
Subject: [PATCH] ALSA: usb-audio: Fix double-free in error paths after
snd_usb_add_audio_stream() call
create_fixed_stream_quirk(), snd_usb_parse_audio_interface() and
create_uaxx_quirk() functions allocate the audioformat object by themselves
and free it upon error before returning. However, once the object is linked
to a stream, it's freed again in snd_usb_audio_pcm_free(), thus it'll be
double-freed, eventually resulting in a memory corruption.
This patch fixes these failures in the error paths by unlinking the audioformat
object before freeing it.
Based on a patch by Takashi Iwai"
[Note for stable backports:
this patch requires the commit 902eb7fd1e4a ('ALSA: usb-audio: Minor
code cleanup in create_fixed_stream_quirk()')]
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
Reported-by: Ralf Spenneberg
Cc: # see the note above
Signed-off-by: Vladis Dronov
---
sound/usb/quirks.c | 4
sound/usb/stream.c | 6 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..6178bb5 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(>list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -193,6 +194,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
return 0;
error:
+ list_del(>list); /* unlink for avoiding double-free */
kfree(fp);
kfree(rate_table);
return err;
@@ -469,6 +471,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
fp->ep_attr = get_endpoint(alts, 0)->bmAttributes;
fp->datainterval = 0;
fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+ INIT_LIST_HEAD(>list);
switch (fp->maxpacksize) {
case 0x120:
@@ -492,6 +495,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(>list); /* unlink for avoiding double-free */
kfree(fp);
return err;
}
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 51258a1..6fe7f21 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -316,7 +316,9 @@ static struct snd_pcm_chmap_elem *convert_chmap(int
channels, unsigned int bits,
/*
* add this endpoint to the chip instance.
* if a stream with the same endpoint already exists, append to it.
- * if not, create a new pcm stream.
+ * if not, create a new pcm stream. note, fp is added to the substream
+ * fmt_list and will be freed on the chip instance release. do not free
+ * fp or do remove it from the substream fmt_list to avoid double-free.
*/
int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
int stream,
@@ -677,6 +679,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio
*chip, int iface_no)
* (fp->maxpacksize & 0x7ff);
fp->attributes = parse_uac_endpoint_attributes(chip, alts,
protocol, iface_no);
fp->clock = clock;
+ INIT_LIST_HEAD(>list);
/* some quirks for attributes here */
@@ -725,6 +728,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio
*chip, int iface_no)
dev_dbg(>dev, "%u:%d: add audio endpoint %#x\n", iface_no,
altno, fp->endpoint);
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(>list); /* unlink for avoiding double-free
*/
kfree(fp->rate_table);
kfree(fp->chmap);
kfree(fp);
--
2.5.5
Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
Hello, Takashi, all, > No, it has nothing to do with the double-free bug itself. Such an > optimization shouldn't be put in a fix patch This piece of code move alone fixes the double-free bug in create_fixed_stream_quirk(), so I believe it is related. Besides, a lot of stuff is created and initialized in snd_usb_add_audio_stream() and while I do not see another use-after-free bug, it could be there. By moving this code we avoid these potential bugs we have not hit yet. But anyway. If you still believe this code should not be moved, please, confirm, I'll suggest the next patch version without it. > Vladis, if you take someone's patch as the base, you have to carry the > original authorship somewhere... Yes, I was thinking about it, I was just not sure how should I do it. Will the following form be fine? Or somehow else? Based on a patch by Takashi Iwai" <ti...@suse.de> > > + * if not, create a new pcm stream. the caller must remove fp from > > + * the substream fmt_list in the error path to avoid double-free. > > This comment isn't true. The caller may leave it as is, too, like my > first version. It just depends on the code. Yes. Is the following rewrite acceptable for the next patch version? * if not, create a new pcm stream. Note, fp is added to the substream fmt_list * and will be freed on the chip instance release. Do not free fp or do remove * it from the substream fmt_list to avoid double-free. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
Hello, Takashi, all, > No, it has nothing to do with the double-free bug itself. Such an > optimization shouldn't be put in a fix patch This piece of code move alone fixes the double-free bug in create_fixed_stream_quirk(), so I believe it is related. Besides, a lot of stuff is created and initialized in snd_usb_add_audio_stream() and while I do not see another use-after-free bug, it could be there. By moving this code we avoid these potential bugs we have not hit yet. But anyway. If you still believe this code should not be moved, please, confirm, I'll suggest the next patch version without it. > Vladis, if you take someone's patch as the base, you have to carry the > original authorship somewhere... Yes, I was thinking about it, I was just not sure how should I do it. Will the following form be fine? Or somehow else? Based on a patch by Takashi Iwai" > > + * if not, create a new pcm stream. the caller must remove fp from > > + * the substream fmt_list in the error path to avoid double-free. > > This comment isn't true. The caller may leave it as is, too, like my > first version. It just depends on the code. Yes. Is the following rewrite acceptable for the next patch version? * if not, create a new pcm stream. Note, fp is added to the substream fmt_list * and will be freed on the chip instance release. Do not free fp or do remove * it from the substream fmt_list to avoid double-free. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
Hello, Takashi, all,
> > Thanks for the report. But how about a simpler fix like below?
>
> Maybe the one below is more straightforward (and even simpler).
> Let me know if this works enough for you.
1) I would still suggest moving the code in create_fixed_stream_quirk() (marked
as (*)) after "if (altsd->bNumEndpoints < 1)" check. This way no allocations is
done in snd_usb_add_audio_stream() if we go an error path:
(*) stream = (fp->endpoint & USB_DIR_IN)
(*) ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
(*) err = snd_usb_add_audio_stream(chip, stream, fp);
(*) if (err < 0)
(*) goto error;
2) While your fix is indeed simpler, it fixes double-free bug only in
create_fixed_stream_quirk(). create_uaxx_quirk(), for example, still has this
bug:
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) { <<< in the error path there
kfree(fp); <<< is a double-free
return err;
}
as any other code where snd_usb_add_audio_stream() is used and *fp is freed in
case of error.
3) Not deleting fp from the fmt_list inside snd_usb_add_audio_stream() in case
of error moves this necessity to a caller, thus breaking the scope. This forces
any caller of snd_usb_add_audio_stream() to fulfill this non-obvious
requirement.
But I agree that this is simpler and more straightforward. We need just to fix
all the places where snd_usb_add_audio_stream() is called (3 as of now), please,
have a look on the following patch.
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
-- 8< --
From: Vladis Dronov <vdro...@redhat.com>
Subject: [PATCH] ALSA: usb-audio: Fix double-free in error paths after
snd_usb_add_audio_stream() call
create_fixed_stream_quirk(), snd_usb_parse_audio_interface() and
create_uaxx_quirk() functions allocate the audioformat object by themselves
and free it upon error before returning. However, once the object is linked
to a stream, it's freed again in snd_usb_audio_pcm_free(), thus it'll be
double-freed, eventually resulting in a memory corruption.
This patch fixes these failures in the error paths by unlinking the audioformat
object before freeing it. Also it moves a piece of code in
create_fixed_stream_quirk() to avoid unnecessary allocations in case of error.
[Note for stable backports:
this patch requires the commit 902eb7fd1e4a ('ALSA: usb-audio: Minor
code cleanup in create_fixed_stream_quirk()')]
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
Reported-by: Ralf Spenneberg <r...@spenneberg.net>
Cc: <sta...@vger.kernel.org> # see the note above
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
sound/usb/quirks.c | 15 ++-
sound/usb/stream.c | 5 -
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..dda5682 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(>list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -164,11 +165,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
fp->rate_table = rate_table;
}
- stream = (fp->endpoint & USB_DIR_IN)
- ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
- err = snd_usb_add_audio_stream(chip, stream, fp);
- if (err < 0)
- goto error;
if (fp->iface !=
get_iface_desc(>altsetting[0])->bInterfaceNumber ||
fp->altset_idx >= iface->num_altsetting) {
err = -EINVAL;
@@ -181,6 +177,12 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
goto error;
}
+ stream = (fp->endpoint & USB_DIR_IN)
+ ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+ err = snd_usb_add_audio_stream(chip, stream, fp);
+ if (err < 0)
+ goto error;
+
fp->protocol = altsd->bInterfaceProtocol;
if (fp->datainterval == 0)
@@ -193,6 +195,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
return 0;
error:
+ list_del(>list); /* unlink for avoiding double-free */
kfree(fp);
kfree(rate_table);
return err;
@@ -469,6 +472,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
fp->ep_attr = get_endpoint(alts, 0)->bmAttributes;
fp->datainterval = 0;
fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+ INIT_LIST_HEAD(>lis
Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
Hello, Takashi, all,
> > Thanks for the report. But how about a simpler fix like below?
>
> Maybe the one below is more straightforward (and even simpler).
> Let me know if this works enough for you.
1) I would still suggest moving the code in create_fixed_stream_quirk() (marked
as (*)) after "if (altsd->bNumEndpoints < 1)" check. This way no allocations is
done in snd_usb_add_audio_stream() if we go an error path:
(*) stream = (fp->endpoint & USB_DIR_IN)
(*) ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
(*) err = snd_usb_add_audio_stream(chip, stream, fp);
(*) if (err < 0)
(*) goto error;
2) While your fix is indeed simpler, it fixes double-free bug only in
create_fixed_stream_quirk(). create_uaxx_quirk(), for example, still has this
bug:
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) { <<< in the error path there
kfree(fp); <<< is a double-free
return err;
}
as any other code where snd_usb_add_audio_stream() is used and *fp is freed in
case of error.
3) Not deleting fp from the fmt_list inside snd_usb_add_audio_stream() in case
of error moves this necessity to a caller, thus breaking the scope. This forces
any caller of snd_usb_add_audio_stream() to fulfill this non-obvious
requirement.
But I agree that this is simpler and more straightforward. We need just to fix
all the places where snd_usb_add_audio_stream() is called (3 as of now), please,
have a look on the following patch.
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
-- 8< --
From: Vladis Dronov
Subject: [PATCH] ALSA: usb-audio: Fix double-free in error paths after
snd_usb_add_audio_stream() call
create_fixed_stream_quirk(), snd_usb_parse_audio_interface() and
create_uaxx_quirk() functions allocate the audioformat object by themselves
and free it upon error before returning. However, once the object is linked
to a stream, it's freed again in snd_usb_audio_pcm_free(), thus it'll be
double-freed, eventually resulting in a memory corruption.
This patch fixes these failures in the error paths by unlinking the audioformat
object before freeing it. Also it moves a piece of code in
create_fixed_stream_quirk() to avoid unnecessary allocations in case of error.
[Note for stable backports:
this patch requires the commit 902eb7fd1e4a ('ALSA: usb-audio: Minor
code cleanup in create_fixed_stream_quirk()')]
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
Reported-by: Ralf Spenneberg
Cc: # see the note above
Signed-off-by: Vladis Dronov
---
sound/usb/quirks.c | 15 ++-
sound/usb/stream.c | 5 -
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..dda5682 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(>list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -164,11 +165,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
fp->rate_table = rate_table;
}
- stream = (fp->endpoint & USB_DIR_IN)
- ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
- err = snd_usb_add_audio_stream(chip, stream, fp);
- if (err < 0)
- goto error;
if (fp->iface !=
get_iface_desc(>altsetting[0])->bInterfaceNumber ||
fp->altset_idx >= iface->num_altsetting) {
err = -EINVAL;
@@ -181,6 +177,12 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
goto error;
}
+ stream = (fp->endpoint & USB_DIR_IN)
+ ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+ err = snd_usb_add_audio_stream(chip, stream, fp);
+ if (err < 0)
+ goto error;
+
fp->protocol = altsd->bInterfaceProtocol;
if (fp->datainterval == 0)
@@ -193,6 +195,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
return 0;
error:
+ list_del(>list); /* unlink for avoiding double-free */
kfree(fp);
kfree(rate_table);
return err;
@@ -469,6 +472,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
fp->ep_attr = get_endpoint(alts, 0)->bmAttributes;
fp->datainterval = 0;
fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+ INIT_LIST_HEAD(>list);
switch (fp->maxpacksize) {
case 0x120:
@@ -492,6 +496,7 @@ static int create_uaxx_quirk(struct snd_usb_audi
[PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
ied that all the
callers of snd_usb_add_audio_stream() free *fp in case of error:
$ git grep snd_usb_add_audio_stream
sound/usb/quirks.c: err = snd_usb_add_audio_stream(chip, stream, fp);
> *err = snd_usb_add_audio_stream(chip, stream, fp);
> *if (err < 0)
> *goto error;
> * error:
> *kfree(fp);
> *kfree(rate_table);
> *return err;
sound/usb/quirks.c: err = snd_usb_add_audio_stream(chip, stream, fp);
> *err = snd_usb_add_audio_stream(chip, stream, fp);
> *if (err < 0) {
> *kfree(fp);
> *return err;
> *}
sound/usb/stream.c: err = snd_usb_add_audio_stream(chip, stream,
fp);
> *err = snd_usb_add_audio_stream(chip, stream, fp);
> *if (err < 0) {
> *kfree(fp->rate_table);
> *kfree(fp->chmap);
> *kfree(fp);
> *return err;
> *}
This means that snd_usb_add_audio_stream() should remove *fp from the
substream's fmt_list list on the error path, if it was already added. Such
places are:
> int snd_usb_add_audio_stream(struct snd_usb_audio *chip, int stream, struct
> audioformat *fp)
> {
> struct snd_usb_stream *as;
> struct snd_usb_substream *subs;
> struct snd_pcm *pcm;
> int err;
>
> list_for_each_entry(as, >pcm_list, list) {
> subs = >substream[stream];
> ...
> list_add_tail(>list, >fmt_list); <<< ADDING
> fp HERE
> subs->num_formats++;
> return 0; <<< NO
> ERROR PATH HERE
> }
> }
>
> /* look for an empty stream */
> list_for_each_entry(as, >pcm_list, list) {
> subs = >substream[stream];
> ...
> snd_usb_init_substream(as, stream, fp); <<< ADDING fp HERE,
> IF add_chmap() FAILS
> return add_chmap(as->pcm, stream, subs); <<< fp SHOULD BE
> REMOVED FROM fmt_list
> }
>
> /* create a new pcm */
> as = kzalloc(sizeof(*as), GFP_KERNEL);
> err = snd_pcm_new(chip->card, "USB Audio", chip->pcm_devs,
> ...
> if (err < 0) {<<< fp IS NOT ADDED YET HERE, SO FINE
> kfree(as);
> return err;
> }
> ...
> snd_usb_init_substream(as, stream, fp); <<< ADDING fp
> HERE
> ... <<< IF
> add_chmap() FAILS fp SHOULD
> return add_chmap(pcm, stream, >substream[stream]); <<< BE
> REMOVED FROM fmt_list
> }
add_chmap() itself does not add anything to fmt_list list, so we indeed need to
remove only the single list element from the list. Having all the above in
mind, the patch follows.
4.3) How to handle possible error paths after successful call to
snd_usb_add_audio_stream() in create_fixed_stream_quirk() is d
iscussable. Properly it should be like the below, but I believe it is
overcomplication here would and stick to a simple error_after_add_audio_stream:
label:
> error2:
> snd_usb_del_audio_stream(...something...);
> error:
> kfree(fp);
> kfree(rate_table);
> return err;
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
[PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
ied that all the
callers of snd_usb_add_audio_stream() free *fp in case of error:
$ git grep snd_usb_add_audio_stream
sound/usb/quirks.c: err = snd_usb_add_audio_stream(chip, stream, fp);
> *err = snd_usb_add_audio_stream(chip, stream, fp);
> *if (err < 0)
> *goto error;
> * error:
> *kfree(fp);
> *kfree(rate_table);
> *return err;
sound/usb/quirks.c: err = snd_usb_add_audio_stream(chip, stream, fp);
> *err = snd_usb_add_audio_stream(chip, stream, fp);
> *if (err < 0) {
> *kfree(fp);
> *return err;
> *}
sound/usb/stream.c: err = snd_usb_add_audio_stream(chip, stream,
fp);
> *err = snd_usb_add_audio_stream(chip, stream, fp);
> *if (err < 0) {
> *kfree(fp->rate_table);
> *kfree(fp->chmap);
> *kfree(fp);
> *return err;
> *}
This means that snd_usb_add_audio_stream() should remove *fp from the
substream's fmt_list list on the error path, if it was already added. Such
places are:
> int snd_usb_add_audio_stream(struct snd_usb_audio *chip, int stream, struct
> audioformat *fp)
> {
> struct snd_usb_stream *as;
> struct snd_usb_substream *subs;
> struct snd_pcm *pcm;
> int err;
>
> list_for_each_entry(as, >pcm_list, list) {
> subs = >substream[stream];
> ...
> list_add_tail(>list, >fmt_list); <<< ADDING
> fp HERE
> subs->num_formats++;
> return 0; <<< NO
> ERROR PATH HERE
> }
> }
>
> /* look for an empty stream */
> list_for_each_entry(as, >pcm_list, list) {
> subs = >substream[stream];
> ...
> snd_usb_init_substream(as, stream, fp); <<< ADDING fp HERE,
> IF add_chmap() FAILS
> return add_chmap(as->pcm, stream, subs); <<< fp SHOULD BE
> REMOVED FROM fmt_list
> }
>
> /* create a new pcm */
> as = kzalloc(sizeof(*as), GFP_KERNEL);
> err = snd_pcm_new(chip->card, "USB Audio", chip->pcm_devs,
> ...
> if (err < 0) {<<< fp IS NOT ADDED YET HERE, SO FINE
> kfree(as);
> return err;
> }
> ...
> snd_usb_init_substream(as, stream, fp); <<< ADDING fp
> HERE
> ... <<< IF
> add_chmap() FAILS fp SHOULD
> return add_chmap(pcm, stream, >substream[stream]); <<< BE
> REMOVED FROM fmt_list
> }
add_chmap() itself does not add anything to fmt_list list, so we indeed need to
remove only the single list element from the list. Having all the above in
mind, the patch follows.
4.3) How to handle possible error paths after successful call to
snd_usb_add_audio_stream() in create_fixed_stream_quirk() is d
iscussable. Properly it should be like the below, but I believe it is
overcomplication here would and stick to a simple error_after_add_audio_stream:
label:
> error2:
> snd_usb_del_audio_stream(...something...);
> error:
> kfree(fp);
> kfree(rate_table);
> return err;
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
[PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
There is a double-free bug in [snd-usb-audio] module due to alloc/free logic
flaw in snd_usb_add_audio_stream() function. This leads to kernel structures
corruption and panic. Fix the code flow and alloc/free logic so there is no
double-free.
The detailed analysis: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
Reported-by: Ralf Spenneberg <r...@spenneberg.net>
Signed-off-by: Vladis Dronov <vdro...@redhat.com>
---
sound/usb/quirks.c | 17 -
sound/usb/stream.c | 10 --
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..1d41b47 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -164,11 +164,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
fp->rate_table = rate_table;
}
- stream = (fp->endpoint & USB_DIR_IN)
- ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
- err = snd_usb_add_audio_stream(chip, stream, fp);
- if (err < 0)
- goto error;
if (fp->iface !=
get_iface_desc(>altsetting[0])->bInterfaceNumber ||
fp->altset_idx >= iface->num_altsetting) {
err = -EINVAL;
@@ -181,6 +176,17 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
goto error;
}
+ stream = (fp->endpoint & USB_DIR_IN)
+ ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+ err = snd_usb_add_audio_stream(chip, stream, fp);
+ if (err < 0)
+ goto error;
+
+ /* From this point error paths should jump to
+* error_after_add_audio_stream: not to error: as fp
+* and rate_table will be freed on stream removal
+*/
+
fp->protocol = altsd->bInterfaceProtocol;
if (fp->datainterval == 0)
@@ -195,6 +201,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
error:
kfree(fp);
kfree(rate_table);
+ error_after_add_audio_stream:
return err;
}
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 51258a1..f8ed8b49 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -349,7 +349,10 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
if (err < 0)
return err;
snd_usb_init_substream(as, stream, fp);
- return add_chmap(as->pcm, stream, subs);
+ err = add_chmap(as->pcm, stream, subs);
+ if (err < 0)
+ list_del_init(>list);
+ return err;
}
/* create a new pcm */
@@ -391,7 +394,10 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
snd_usb_proc_pcm_format_add(as);
- return add_chmap(pcm, stream, >substream[stream]);
+ err = add_chmap(pcm, stream, >substream[stream]);
+ if (err < 0)
+ list_del_init(>list);
+ return err;
}
static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
--
2.5.5
[PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()
There is a double-free bug in [snd-usb-audio] module due to alloc/free logic
flaw in snd_usb_add_audio_stream() function. This leads to kernel structures
corruption and panic. Fix the code flow and alloc/free logic so there is no
double-free.
The detailed analysis: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
Reported-by: Ralf Spenneberg
Signed-off-by: Vladis Dronov
---
sound/usb/quirks.c | 17 -
sound/usb/stream.c | 10 --
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..1d41b47 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -164,11 +164,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
fp->rate_table = rate_table;
}
- stream = (fp->endpoint & USB_DIR_IN)
- ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
- err = snd_usb_add_audio_stream(chip, stream, fp);
- if (err < 0)
- goto error;
if (fp->iface !=
get_iface_desc(>altsetting[0])->bInterfaceNumber ||
fp->altset_idx >= iface->num_altsetting) {
err = -EINVAL;
@@ -181,6 +176,17 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
goto error;
}
+ stream = (fp->endpoint & USB_DIR_IN)
+ ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+ err = snd_usb_add_audio_stream(chip, stream, fp);
+ if (err < 0)
+ goto error;
+
+ /* From this point error paths should jump to
+* error_after_add_audio_stream: not to error: as fp
+* and rate_table will be freed on stream removal
+*/
+
fp->protocol = altsd->bInterfaceProtocol;
if (fp->datainterval == 0)
@@ -195,6 +201,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio
*chip,
error:
kfree(fp);
kfree(rate_table);
+ error_after_add_audio_stream:
return err;
}
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 51258a1..f8ed8b49 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -349,7 +349,10 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
if (err < 0)
return err;
snd_usb_init_substream(as, stream, fp);
- return add_chmap(as->pcm, stream, subs);
+ err = add_chmap(as->pcm, stream, subs);
+ if (err < 0)
+ list_del_init(>list);
+ return err;
}
/* create a new pcm */
@@ -391,7 +394,10 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
snd_usb_proc_pcm_format_add(as);
- return add_chmap(pcm, stream, >substream[stream]);
+ err = add_chmap(pcm, stream, >substream[stream]);
+ if (err < 0)
+ list_del_init(>list);
+ return err;
}
static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
--
2.5.5
