2019-01-30 05:39:38

by Myungho Jung

[permalink] [raw]
Subject: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

tiocmget() and tiocmset() operations are optional and some tty drivers
like pty miss the operations. We need NULL check to prevent from
dereference.

Signed-off-by: Myungho Jung <[email protected]>
---
drivers/bluetooth/hci_ath.c | 6 ++++++
drivers/bluetooth/hci_ldisc.c | 4 ++++
2 files changed, 10 insertions(+)

diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index d568fbd94d6c..fb9f6323a911 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -185,8 +185,14 @@ static int ath_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)

static int ath_setup(struct hci_uart *hu)
{
+ struct tty_struct *tty = hu->tty;
+
BT_DBG("hu %p", hu);

+ /* tty driver should support operations to set RTS */
+ if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
+ return -EOPNOTSUPP;
+
hu->hdev->set_bdaddr = ath_set_bdaddr;

return 0;
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fbf7b4df23ab..cb31c2d8d826 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -314,6 +314,10 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
return;
}

+ /* tiocmget() and tiocmset() operations are optional */
+ if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
+ return;
+
if (enable) {
/* Disable hardware flow control */
ktermios = tty->termios;
--
2.17.1



2019-01-31 15:40:11

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

On Tue, Jan 29, 2019 at 09:39:28PM -0800, Myungho Jung wrote:
> tiocmget() and tiocmset() operations are optional and some tty drivers
> like pty miss the operations. We need NULL check to prevent from
> dereference.
>
> Signed-off-by: Myungho Jung <[email protected]>
> ---
> drivers/bluetooth/hci_ath.c | 6 ++++++
> drivers/bluetooth/hci_ldisc.c | 4 ++++
> 2 files changed, 10 insertions(+)

Ah, you had already submitted a v2.

I still suggest splitting this one in two patches and that you add a
Fixes and stable tag to each so that they both get backported to stable.

Also, when resubmitting, make sure to include a short changelog here
below the cut-off line (---).

>
> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> index d568fbd94d6c..fb9f6323a911 100644
> --- a/drivers/bluetooth/hci_ath.c
> +++ b/drivers/bluetooth/hci_ath.c
> @@ -185,8 +185,14 @@ static int ath_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>
> static int ath_setup(struct hci_uart *hu)
> {
> + struct tty_struct *tty = hu->tty;
> +
> BT_DBG("hu %p", hu);
>
> + /* tty driver should support operations to set RTS */
> + if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
> + return -EOPNOTSUPP;

-ENODEV might be more appropriate.

Johan

2019-02-03 06:38:34

by Myungho Jung

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

On Thu, Jan 31, 2019 at 04:40:00PM +0100, Johan Hovold wrote:
> On Tue, Jan 29, 2019 at 09:39:28PM -0800, Myungho Jung wrote:
> > tiocmget() and tiocmset() operations are optional and some tty drivers
> > like pty miss the operations. We need NULL check to prevent from
> > dereference.
> >
> > Signed-off-by: Myungho Jung <[email protected]>
> > ---
> > drivers/bluetooth/hci_ath.c | 6 ++++++
> > drivers/bluetooth/hci_ldisc.c | 4 ++++
> > 2 files changed, 10 insertions(+)
>
> Ah, you had already submitted a v2.
>
> I still suggest splitting this one in two patches and that you add a
> Fixes and stable tag to each so that they both get backported to stable.
>
> Also, when resubmitting, make sure to include a short changelog here
> below the cut-off line (---).
>
> >
> > diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> > index d568fbd94d6c..fb9f6323a911 100644
> > --- a/drivers/bluetooth/hci_ath.c
> > +++ b/drivers/bluetooth/hci_ath.c
> > @@ -185,8 +185,14 @@ static int ath_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> >
> > static int ath_setup(struct hci_uart *hu)
> > {
> > + struct tty_struct *tty = hu->tty;
> > +
> > BT_DBG("hu %p", hu);
> >
> > + /* tty driver should support operations to set RTS */
> > + if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
> > + return -EOPNOTSUPP;
>
> -ENODEV might be more appropriate.
>
> Johan

I'll split into 2 seperate patches. So, do I need to add stable tag on each
patch like below to be cherry-picked?

Cc: <[email protected]> # <hash>: <summary>

I looked for a good example from mailing list but didn't find one.

Thanks,
Myungho


2019-02-03 10:53:32

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

On Sat, Feb 02, 2019 at 10:38:24PM -0800, Myungho Jung wrote:
> On Thu, Jan 31, 2019 at 04:40:00PM +0100, Johan Hovold wrote:
> > On Tue, Jan 29, 2019 at 09:39:28PM -0800, Myungho Jung wrote:
> > > tiocmget() and tiocmset() operations are optional and some tty drivers
> > > like pty miss the operations. We need NULL check to prevent from
> > > dereference.
> > >
> > > Signed-off-by: Myungho Jung <[email protected]>
> > > ---
> > > drivers/bluetooth/hci_ath.c | 6 ++++++
> > > drivers/bluetooth/hci_ldisc.c | 4 ++++
> > > 2 files changed, 10 insertions(+)
> >
> > Ah, you had already submitted a v2.
> >
> > I still suggest splitting this one in two patches and that you add a
> > Fixes and stable tag to each so that they both get backported to stable.
> >
> > Also, when resubmitting, make sure to include a short changelog here
> > below the cut-off line (---).
> >
> > >
> > > diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> > > index d568fbd94d6c..fb9f6323a911 100644
> > > --- a/drivers/bluetooth/hci_ath.c
> > > +++ b/drivers/bluetooth/hci_ath.c
> > > @@ -185,8 +185,14 @@ static int ath_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> > >
> > > static int ath_setup(struct hci_uart *hu)
> > > {
> > > + struct tty_struct *tty = hu->tty;
> > > +
> > > BT_DBG("hu %p", hu);
> > >
> > > + /* tty driver should support operations to set RTS */
> > > + if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
> > > + return -EOPNOTSUPP;
> >
> > -ENODEV might be more appropriate.
> >
> > Johan
>
> I'll split into 2 seperate patches. So, do I need to add stable tag on each
> patch like below to be cherry-picked?
>
> Cc: <[email protected]> # <hash>: <summary>
>
> I looked for a good example from mailing list but didn't find one.

Almost right, the format you use above is actually used to identify
dependencies for backports.

You should add a Fixes tag identifying the commit which introduced each
bug and a stable-cc tag. If you want you can indicate the version after
a # sign, but that can also be inferred from the fixes tag.

For the hci_ldisc fix I think the tags would be:

Fixes: 2a973dfada2b ("Bluetooth: hci_uart: Add new line discipline enhancements")
Cc: stable <[email protected]> # 4.2

You can use git blame to track down the offending commits.

This should all be explained here:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Johan

2019-02-04 07:29:06

by Myungho Jung

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

On Sun, Feb 03, 2019 at 11:53:23AM +0100, Johan Hovold wrote:
> On Sat, Feb 02, 2019 at 10:38:24PM -0800, Myungho Jung wrote:
> > On Thu, Jan 31, 2019 at 04:40:00PM +0100, Johan Hovold wrote:
> > > On Tue, Jan 29, 2019 at 09:39:28PM -0800, Myungho Jung wrote:
> > > > tiocmget() and tiocmset() operations are optional and some tty drivers
> > > > like pty miss the operations. We need NULL check to prevent from
> > > > dereference.
> > > >
> > > > Signed-off-by: Myungho Jung <[email protected]>
> > > > ---
> > > > drivers/bluetooth/hci_ath.c | 6 ++++++
> > > > drivers/bluetooth/hci_ldisc.c | 4 ++++
> > > > 2 files changed, 10 insertions(+)
> > >
> > > Ah, you had already submitted a v2.
> > >
> > > I still suggest splitting this one in two patches and that you add a
> > > Fixes and stable tag to each so that they both get backported to stable.
> > >
> > > Also, when resubmitting, make sure to include a short changelog here
> > > below the cut-off line (---).
> > >
> > > >
> > > > diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> > > > index d568fbd94d6c..fb9f6323a911 100644
> > > > --- a/drivers/bluetooth/hci_ath.c
> > > > +++ b/drivers/bluetooth/hci_ath.c
> > > > @@ -185,8 +185,14 @@ static int ath_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> > > >
> > > > static int ath_setup(struct hci_uart *hu)
> > > > {
> > > > + struct tty_struct *tty = hu->tty;
> > > > +
> > > > BT_DBG("hu %p", hu);
> > > >
> > > > + /* tty driver should support operations to set RTS */
> > > > + if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
> > > > + return -EOPNOTSUPP;
> > >
> > > -ENODEV might be more appropriate.
> > >
> > > Johan
> >
> > I'll split into 2 seperate patches. So, do I need to add stable tag on each
> > patch like below to be cherry-picked?
> >
> > Cc: <[email protected]> # <hash>: <summary>
> >
> > I looked for a good example from mailing list but didn't find one.
>
> Almost right, the format you use above is actually used to identify
> dependencies for backports.
>
> You should add a Fixes tag identifying the commit which introduced each
> bug and a stable-cc tag. If you want you can indicate the version after
> a # sign, but that can also be inferred from the fixes tag.
>
> For the hci_ldisc fix I think the tags would be:
>
> Fixes: 2a973dfada2b ("Bluetooth: hci_uart: Add new line discipline enhancements")
> Cc: stable <[email protected]> # 4.2
>
> You can use git blame to track down the offending commits.
>
> This should all be explained here:
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> Johan

Thank you for the explanation. I'll carefully read the page and submit the next
patch.

Thanks,
Myungho

2019-02-04 09:04:47

by Myungho Jung

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

On Sun, Feb 03, 2019 at 11:29:00PM -0800, Myungho Jung wrote:
> On Sun, Feb 03, 2019 at 11:53:23AM +0100, Johan Hovold wrote:
> > On Sat, Feb 02, 2019 at 10:38:24PM -0800, Myungho Jung wrote:
> > > On Thu, Jan 31, 2019 at 04:40:00PM +0100, Johan Hovold wrote:
> > > > On Tue, Jan 29, 2019 at 09:39:28PM -0800, Myungho Jung wrote:
> > > > > tiocmget() and tiocmset() operations are optional and some tty drivers
> > > > > like pty miss the operations. We need NULL check to prevent from
> > > > > dereference.
> > > > >
> > > > > Signed-off-by: Myungho Jung <[email protected]>
> > > > > ---
> > > > > drivers/bluetooth/hci_ath.c | 6 ++++++
> > > > > drivers/bluetooth/hci_ldisc.c | 4 ++++
> > > > > 2 files changed, 10 insertions(+)
> > > >
> > > > Ah, you had already submitted a v2.
> > > >
> > > > I still suggest splitting this one in two patches and that you add a
> > > > Fixes and stable tag to each so that they both get backported to stable.
> > > >
> > > > Also, when resubmitting, make sure to include a short changelog here
> > > > below the cut-off line (---).
> > > >
> > > > >
> > > > > diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> > > > > index d568fbd94d6c..fb9f6323a911 100644
> > > > > --- a/drivers/bluetooth/hci_ath.c
> > > > > +++ b/drivers/bluetooth/hci_ath.c
> > > > > @@ -185,8 +185,14 @@ static int ath_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> > > > >
> > > > > static int ath_setup(struct hci_uart *hu)
> > > > > {
> > > > > + struct tty_struct *tty = hu->tty;
> > > > > +
> > > > > BT_DBG("hu %p", hu);
> > > > >
> > > > > + /* tty driver should support operations to set RTS */
> > > > > + if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
> > > > > + return -EOPNOTSUPP;
> > > >
> > > > -ENODEV might be more appropriate.
> > > >
> > > > Johan
> > >
> > > I'll split into 2 seperate patches. So, do I need to add stable tag on each
> > > patch like below to be cherry-picked?
> > >
> > > Cc: <[email protected]> # <hash>: <summary>
> > >
> > > I looked for a good example from mailing list but didn't find one.
> >
> > Almost right, the format you use above is actually used to identify
> > dependencies for backports.
> >
> > You should add a Fixes tag identifying the commit which introduced each
> > bug and a stable-cc tag. If you want you can indicate the version after
> > a # sign, but that can also be inferred from the fixes tag.
> >
> > For the hci_ldisc fix I think the tags would be:
> >
> > Fixes: 2a973dfada2b ("Bluetooth: hci_uart: Add new line discipline enhancements")
> > Cc: stable <[email protected]> # 4.2
> >
> > You can use git blame to track down the offending commits.
> >
> > This should all be explained here:
> >
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> >
> > Johan
>
> Thank you for the explanation. I'll carefully read the page and submit the next
> patch.
>
> Thanks,
> Myungho

Hi Johan,

I have one more question. The title of new patches should be version 3 like
this?
[PATCH v3] Bluetooth: hci_ath: ...
[PATCH v3] Bluetooth: hci_ldisc: ...

Or newly start with [PATCH]?

Thanks,
Myungho


2019-02-04 09:22:21

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

On Mon, Feb 04, 2019 at 01:04:37AM -0800, Myungho Jung wrote:
> On Sun, Feb 03, 2019 at 11:29:00PM -0800, Myungho Jung wrote:
> > On Sun, Feb 03, 2019 at 11:53:23AM +0100, Johan Hovold wrote:

> > > You should add a Fixes tag identifying the commit which introduced each
> > > bug and a stable-cc tag. If you want you can indicate the version after
> > > a # sign, but that can also be inferred from the fixes tag.
> > >
> > > For the hci_ldisc fix I think the tags would be:
> > >
> > > Fixes: 2a973dfada2b ("Bluetooth: hci_uart: Add new line discipline enhancements")
> > > Cc: stable <[email protected]> # 4.2
> > >
> > > You can use git blame to track down the offending commits.
> > >
> > > This should all be explained here:
> > >
> > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> I have one more question. The title of new patches should be version 3 like
> this?
> [PATCH v3] Bluetooth: hci_ath: ...
> [PATCH v3] Bluetooth: hci_ldisc: ...
>
> Or newly start with [PATCH]?

You should keep and increment the revision number even if you split a
patch (so use v3 for the resend).

I suggest you send both patches in a series (as they are related); take
a look at git-send-email for a convenient way to do that.

And always include a brief changelog (below the cut-off line, "---")
when revising patches.

Johan

2019-02-04 09:30:06

by Myungho Jung

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

On Mon, Feb 04, 2019 at 10:22:16AM +0100, Johan Hovold wrote:
> On Mon, Feb 04, 2019 at 01:04:37AM -0800, Myungho Jung wrote:
> > On Sun, Feb 03, 2019 at 11:29:00PM -0800, Myungho Jung wrote:
> > > On Sun, Feb 03, 2019 at 11:53:23AM +0100, Johan Hovold wrote:
>
> > > > You should add a Fixes tag identifying the commit which introduced each
> > > > bug and a stable-cc tag. If you want you can indicate the version after
> > > > a # sign, but that can also be inferred from the fixes tag.
> > > >
> > > > For the hci_ldisc fix I think the tags would be:
> > > >
> > > > Fixes: 2a973dfada2b ("Bluetooth: hci_uart: Add new line discipline enhancements")
> > > > Cc: stable <[email protected]> # 4.2
> > > >
> > > > You can use git blame to track down the offending commits.
> > > >
> > > > This should all be explained here:
> > > >
> > > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> > I have one more question. The title of new patches should be version 3 like
> > this?
> > [PATCH v3] Bluetooth: hci_ath: ...
> > [PATCH v3] Bluetooth: hci_ldisc: ...
> >
> > Or newly start with [PATCH]?
>
> You should keep and increment the revision number even if you split a
> patch (so use v3 for the resend).
>
> I suggest you send both patches in a series (as they are related); take
> a look at git-send-email for a convenient way to do that.
>
> And always include a brief changelog (below the cut-off line, "---")
> when revising patches.
>
> Johan

I see. I'll submit version 3 as a patchset with change logs.

Thanks,
Myungho