From: Kefeng Wang <[email protected]>
commit 56897b217a1d0a91c9920cb418d6b3fe922f590a upstream.
task A: task B:
hci_uart_set_proto flush_to_ldisc
- p->open(hu) -> h5_open //alloc h5 - receive_buf
- set_bit HCI_UART_PROTO_READY - tty_port_default_receive_buf
- hci_uart_register_dev - tty_ldisc_receive_buf
- hci_uart_tty_receive
- test_bit HCI_UART_PROTO_READY
- h5_recv
- clear_bit HCI_UART_PROTO_READY while() {
- p->open(hu) -> h5_close //free h5
- h5_rx_3wire_hdr
- h5_reset() //use-after-free
}
It could use ioctl to set hci uart proto, but there is
a use-after-free issue when hci_uart_register_dev() fail in
hci_uart_set_proto(), see stack above, fix this by setting
HCI_UART_PROTO_READY bit only when hci_uart_register_dev()
return success.
Reported-by: [email protected]
Signed-off-by: Kefeng Wang <[email protected]>
Reviewed-by: Jeremy Cline <[email protected]>
Signed-off-by: Marcel Holtmann <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -653,15 +653,14 @@ static int hci_uart_set_proto(struct hci
return err;
hu->proto = p;
- set_bit(HCI_UART_PROTO_READY, &hu->flags);
err = hci_uart_register_dev(hu);
if (err) {
- clear_bit(HCI_UART_PROTO_READY, &hu->flags);
p->close(hu);
return err;
}
+ set_bit(HCI_UART_PROTO_READY, &hu->flags);
return 0;
}
Hi Greg,
On Fri, Nov 15, 2019 at 02:20:31PM +0800, Greg Kroah-Hartman wrote:
>From: Kefeng Wang <[email protected]>
>
>commit 56897b217a1d0a91c9920cb418d6b3fe922f590a upstream.
>
>task A: task B:
>hci_uart_set_proto flush_to_ldisc
> - p->open(hu) -> h5_open //alloc h5 - receive_buf
> - set_bit HCI_UART_PROTO_READY - tty_port_default_receive_buf
> - hci_uart_register_dev - tty_ldisc_receive_buf
> - hci_uart_tty_receive
> - test_bit HCI_UART_PROTO_READY
> - h5_recv
> - clear_bit HCI_UART_PROTO_READY while() {
> - p->open(hu) -> h5_close //free h5
> - h5_rx_3wire_hdr
> - h5_reset() //use-after-free
> }
>
>It could use ioctl to set hci uart proto, but there is
>a use-after-free issue when hci_uart_register_dev() fail in
>hci_uart_set_proto(), see stack above, fix this by setting
>HCI_UART_PROTO_READY bit only when hci_uart_register_dev()
>return success.
>
>Reported-by: [email protected]
>Signed-off-by: Kefeng Wang <[email protected]>
>Reviewed-by: Jeremy Cline <[email protected]>
>Signed-off-by: Marcel Holtmann <[email protected]>
>Signed-off-by: Greg Kroah-Hartman <[email protected]>
I was just about to ask why this had not been merged into 4.9. Spent a
while searching archives for any discussion to explain its absence, but
couldn't find anything. Also watched your kernel-recipes talk...
BTW, this also seems to be missing from 4.4 branch, although it was
merged for 3.16 (per https://lore.kernel.org/stable/?q=Postpone+HCI).
I gather that the usual rule is that a fix must be in newer versions
before it can go into older ones. Or at least, some patches were
rejected on that basis. If this is in fact the policy, perhaps it could
be added to stable-kernel-rules.rst ?
-Ralph
>---
> drivers/bluetooth/hci_ldisc.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
>--- a/drivers/bluetooth/hci_ldisc.c
>+++ b/drivers/bluetooth/hci_ldisc.c
>@@ -653,15 +653,14 @@ static int hci_uart_set_proto(struct hci
> return err;
>
> hu->proto = p;
>- set_bit(HCI_UART_PROTO_READY, &hu->flags);
>
> err = hci_uart_register_dev(hu);
> if (err) {
>- clear_bit(HCI_UART_PROTO_READY, &hu->flags);
> p->close(hu);
> return err;
> }
>
>+ set_bit(HCI_UART_PROTO_READY, &hu->flags);
> return 0;
> }
>
>
>
On Fri, Nov 15, 2019 at 11:10:29AM -0500, Ralph Siemsen wrote:
> Hi Greg,
>
> On Fri, Nov 15, 2019 at 02:20:31PM +0800, Greg Kroah-Hartman wrote:
> > From: Kefeng Wang <[email protected]>
> >
> > commit 56897b217a1d0a91c9920cb418d6b3fe922f590a upstream.
> >
> > task A: task B:
> > hci_uart_set_proto flush_to_ldisc
> > - p->open(hu) -> h5_open //alloc h5 - receive_buf
> > - set_bit HCI_UART_PROTO_READY - tty_port_default_receive_buf
> > - hci_uart_register_dev - tty_ldisc_receive_buf
> > - hci_uart_tty_receive
> > - test_bit HCI_UART_PROTO_READY
> > - h5_recv
> > - clear_bit HCI_UART_PROTO_READY while() {
> > - p->open(hu) -> h5_close //free h5
> > - h5_rx_3wire_hdr
> > - h5_reset() //use-after-free
> > }
> >
> > It could use ioctl to set hci uart proto, but there is
> > a use-after-free issue when hci_uart_register_dev() fail in
> > hci_uart_set_proto(), see stack above, fix this by setting
> > HCI_UART_PROTO_READY bit only when hci_uart_register_dev()
> > return success.
> >
> > Reported-by: [email protected]
> > Signed-off-by: Kefeng Wang <[email protected]>
> > Reviewed-by: Jeremy Cline <[email protected]>
> > Signed-off-by: Marcel Holtmann <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> I was just about to ask why this had not been merged into 4.9. Spent a while
> searching archives for any discussion to explain its absence, but couldn't
> find anything. Also watched your kernel-recipes talk...
>
> BTW, this also seems to be missing from 4.4 branch, although it was merged
> for 3.16 (per https://lore.kernel.org/stable/?q=Postpone+HCI).
Odd that it was merged into 3.16, perhaps it was done there because some
earlier patch added the problem? I say this as I do not think this is
relevant for the 4.4.y kernel, do you? Have you tried to apply this
patch there?
> I gather that the usual rule is that a fix must be in newer versions before
> it can go into older ones. Or at least, some patches were rejected on that
> basis. If this is in fact the policy, perhaps it could be added to
> stable-kernel-rules.rst ?
No, that's not why this was rejected. I don't know why it didn't end up
in 4.9.y earlier, but for 4.4.y, it was not added there as I do not
think it actually is relevant (see above.)
thanks,
greg k-h
On Sat, Nov 16, 2019 at 03:56:14PM +0800, Greg Kroah-Hartman wrote:
>>
>> BTW, this also seems to be missing from 4.4 branch, although it was merged
>> for 3.16 (per https://lore.kernel.org/stable/?q=Postpone+HCI).
>
>Odd that it was merged into 3.16, perhaps it was done there because some
>earlier patch added the problem?
This patch should really be viewed as a correction to an earlier commit:
84cb3df02aea ("Bluetooth: hci_ldisc: Fix null pointer derefence in case
of early data"). This was merged 2016-Apr-08 into v4.7, and therefore is
included in 4.9 and higher.
Only very recently, on 2019-Sep-23, this was backported to 3.16, along
with the correction. Both appeared in v3.16.74.
> I say this as I do not think this is
>relevant for the 4.4.y kernel, do you? Have you tried to apply this
>patch there?
The patch does not apply, but this is mainly due to the earlier commit
missing. It seems to me like that earlier fix is desirable (and it was
put into 3.16), along with the followup. So I would think we want it in
4.4 as well.
[Aside: I'm really only interested in 4.9 and 4.19, so the 4.4 stuff is
just a diversion. But figured I might as well mention what I found...]
Regards,
-Ralph
On Mon, Nov 18, 2019 at 03:27:12PM -0500, Ralph Siemsen wrote:
> On Sat, Nov 16, 2019 at 03:56:14PM +0800, Greg Kroah-Hartman wrote:
> > >
> > > BTW, this also seems to be missing from 4.4 branch, although it was merged
> > > for 3.16 (per https://lore.kernel.org/stable/?q=Postpone+HCI).
> >
> > Odd that it was merged into 3.16, perhaps it was done there because some
> > earlier patch added the problem?
>
> This patch should really be viewed as a correction to an earlier commit:
> 84cb3df02aea ("Bluetooth: hci_ldisc: Fix null pointer derefence in case of
> early data"). This was merged 2016-Apr-08 into v4.7, and therefore is
> included in 4.9 and higher.
>
> Only very recently, on 2019-Sep-23, this was backported to 3.16, along with
> the correction. Both appeared in v3.16.74.
Ok, that makes more sense now. The "fix" didn't apply as it was not a
fix for an old issue, but rather a new one.
> > I say this as I do not think this is
> > relevant for the 4.4.y kernel, do you? Have you tried to apply this
> > patch there?
>
> The patch does not apply, but this is mainly due to the earlier commit
> missing. It seems to me like that earlier fix is desirable (and it was put
> into 3.16), along with the followup. So I would think we want it in 4.4 as
> well.
I've queued them both up now, thanks.
> [Aside: I'm really only interested in 4.9 and 4.19, so the 4.4 stuff is just
> a diversion. But figured I might as well mention what I found...]
Other people at your company care about 4.4.y :)
thanks,
greg k-h