Re: [PATCH] tty: Fix WARNING in tty_set_termios

2019-01-31 Thread Johan Hovold
On Thu, Jan 31, 2019 at 04:18:33PM +0100, Marcel Holtmann wrote:

> > I agree with Al that this change doesn't make much sense. The WARN_ON
> > is there to catch any bugs leading to the termios being changed for a
> > master side pty. Those should bugs should be fixed, and not worked
> > around in order to silence a WARN_ON.
> > 
> > The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
> > operational speed during setup") which introduced a new way for how
> > tty_set_termios() could end up being called for a master pty.
> > 
> > As Al hinted at, setting these ldiscs for a master pty really makes no
> > sense and perhaps that is what we should prevent unless simply making
> > sure they do not call tty_set_termios() is sufficient for the time
> > being.
> > 
> > Finally, note that serdev never operates on a pty, and that this is only
> > an issue for (the three) line disciplines.
> 
> I think for PTYs we should just fail setting the HCI line discipline.
> Fail early and just move on with life.

Sounds good to me. At least for the pty master. There may be some people
trying to use a bluetooth device connected to a remote serial port (I've
seen descriptions of such setups at least), and maybe we need not prevent
that.

Johan


Re: [PATCH] tty: Fix WARNING in tty_set_termios

2019-01-30 Thread Johan Hovold
On Mon, Jan 28, 2019 at 02:29:22PM -0700, shuah wrote:
> On 1/25/19 9:14 PM, Al Viro wrote:
> > On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote:
> >> tty_set_termios() has the following WARMN_ON which can be triggered with a
> >> syscall to invoke TIOCGETD __NR_ioctl.

You meant TIOCSETD here, and in fact its the call which sets the uart
protocol that triggers the warning.

> >> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> >>  tty->driver->subtype == PTY_TYPE_MASTER);
> >> Reference: 
> >> https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
> >>
> >> A simple change would have been to print error message instead of WARN_ON.
> >> However, the callers assume that tty_set_termios() always returns 0 and
> >> don't check return value. The complete solution is fixing all the callers
> >> to check error and bail out to fix the WARN_ON.
> >>
> >> This fix changes tty_set_termios() to return error and all the callers
> >> to check error and bail out. The reproducer is used to reproduce the
> >> problem and verify the fix.
> > 
> >> --- a/drivers/bluetooth/hci_ldisc.c
> >> +++ b/drivers/bluetooth/hci_ldisc.c
> >> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, 
> >> bool enable)
> >>status = tty_set_termios(tty, &ktermios);
> >>BT_DBG("Disabling hardware flow control: %s",
> >>   status ? "failed" : "success");
> >> +  if (status)
> >> +  return;
> > 
> > Can that ldisc end up set on pty master?  And does it make any sense there?
> 
> The initial objective of the patch is to prevent the WARN_ON by making
> the change to return error instead of WARN_ON. However, without changes
> to places that don't check the return and keep making progress, there
> will be secondary problems.
> 
> Without this change to return here, instead of WARN_ON, it will fail
> with the following NULL pointer dereference at the next thing 
> hci_uart_set_flow_control() attempts.
> 
> status = tty->driver->ops->tiocmget(tty);
> 
> kernel: [10140.649783] BUG: unable to handle kernel NULL pointer 

That's a separate issue, which is being fixed:

https://lkml.kernel.org/r/20190130095938.GP3691@localhost

> > IOW, I don't believe that this patch makes any sense.  If anything,
> > we need to prevent unconditional tty_set_termios() on the path
> > that *does* lead to calling it for pty.
> 
> I don't think preventing unconditional tty_set_termios() is enough to
> prevent secondary problems such as the one above.
> 
> For example, the following call chain leads to the WARN_ON that was
> reported. Even if void hci_uart_set_baudrate() prevents the very first
> tty_set_termios() call, its caller hci_uart_setup() continues with
> more tty setup. It goes ahead to call driver setup callback. The
> driver callback goes on to do more setup calling tty_set_termios().
> 
> WARN_ON call path:
>   hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378
>   hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401
>   hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423
> 
> Once this WARN_ON is changed to return error, the following
> happens, when hci_uart_setup() does driver setup callback.
> 
> kernel: [10140.649836]  mrvl_setup+0x17/0x80 [hci_uart]
> kernel: [10140.649840]  hci_uart_setup+0x56/0x160 [hci_uart]
> kernel: [10140.649850]  hci_dev_do_open+0xe6/0x630 [bluetooth]
> kernel: [10140.649860]  hci_power_on+0x52/0x220 [bluetooth]
> 
> I think continuing to catch the invalid condition in tty_set_termios()
> and preventing progress by checking return value is a straight forward
> change to avoid secondary problems, and it might be difficult to catch
> all the cases where it could fail.

I agree with Al that this change doesn't make much sense. The WARN_ON
is there to catch any bugs leading to the termios being changed for a
master side pty. Those should bugs should be fixed, and not worked
around in order to silence a WARN_ON.

The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
operational speed during setup") which introduced a new way for how
tty_set_termios() could end up being called for a master pty.

As Al hinted at, setting these ldiscs for a master pty really makes no
sense and perhaps that is what we should prevent unless simply making
sure they do not call tty_set_termios() is sufficient for the time
being.

Finally, note that serdev never operates on a pty, and that this is only
an issue for (the three) line disciplines.

Johan


Re: [PATCH v4 4/6] nfc: pn533: add UART phy driver

2018-12-05 Thread Johan Hovold
On Mon, Dec 03, 2018 at 03:26:22PM +0100, Lars Poeschel wrote:
> On Wed, Nov 14, 2018 at 04:35:17PM +0100, Johan Hovold wrote:
> > On Thu, Nov 01, 2018 at 11:02:12AM +0100, Lars Poeschel wrote:
> > > This adds the UART phy interface for the pn533 driver.
> > > The pn533 driver can be used through UART interface this way.
> > > It is implemented as a serdev device.
> > > 
> > > Signed-off-by: Lars Poeschel 
> > 
> > Please make sure to include reviewers on CC.
> 
> It's hard to do all things right, about how to correctly email patches,
> patch sets and follow ups and send what to whom.
> I am still learning. Sorry about that.

No problem, I fully understand that.

> > > + err = serdev_device_open(serdev);
> > > + if (err) {
> > > + dev_err(&serdev->dev, "Unable to open device\n");
> > > + goto err_skb;
> > > + }
> > > +
> > > + err = serdev_device_set_baudrate(serdev, 115200);
> > > + if (err != 115200) {
> > > + err = -EINVAL;
> > > + goto err_serdev;
> > > + }
> > > +
> > > + serdev_device_set_flow_control(serdev, false);
> > > + pn532->send_wakeup = PN532_SEND_WAKEUP;
> > > + timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
> > > + priv = pn533_register_device(PN533_DEVICE_PN532,
> > > +  PN533_NO_TYPE_B_PROTOCOLS,
> > > +  PN533_PROTO_REQ_ACK_RESP,
> > > +  pn532, &uart_phy_ops, NULL,
> > > +  &pn532->serdev->dev,
> > > +  &serdev->dev);
> > > + if (IS_ERR(priv)) {
> > > + err = PTR_ERR(priv);
> > > + goto err_serdev;
> > > + }
> > > +
> > > + pn532->priv = priv;
> > > + err = pn533_finalize_setup(pn532->priv);
> > > + if (err)
> > > + goto err_unregister;
> > > +
> > > + serdev_device_close(serdev);
> > 
> > This looks broken; what if the NFC interface is brought up before this
> > point? You'd get a double open, which is likely to crash things, but
> > even if you survive that, the port would not be closed despite the
> > interface being up.
> 
> I understand the problem and would like to solve it with a mutex. I will
> not have time to work on that until next year. Please be patient. I will
> send a new patchset.

I'm in no hurry here. :)

But I still think doing that setup before registering the device might
be preferred if it's possible as you wouldn't need a mutex then.

> > Can't you finalise your setup before registering the interface?
> 
> Well, propably I can do that. But I did it the way the other drivers
> (usb and i2c) are already doing and reusing the code of the pn533 core
> driver. Since their probe works very similar to mine, I suspect them to
> have the same problems.

Quite possibly.

> I can rewrite the probe for my driver, but not for the other two. I can
> not test them.
> Would you prefer that I rewrite my own _register_device and
> _finalize_setup functions, not using the ones from the core driver ?

If you add common paths that you can test using your driver I think it
should be fine to convert the others unless it ends up being really
complicated. Perhaps the authors of those driver can help out with
testing too.

> Thank you for your very valuable feedback.

You're welcome.

Johan


Re: [PATCH v4 4/6] nfc: pn533: add UART phy driver

2018-11-14 Thread Johan Hovold
On Thu, Nov 01, 2018 at 11:02:12AM +0100, Lars Poeschel wrote:
> This adds the UART phy interface for the pn533 driver.
> The pn533 driver can be used through UART interface this way.
> It is implemented as a serdev device.
> 
> Signed-off-by: Lars Poeschel 

Please make sure to include reviewers on CC.

> ---
> Changes in v4:
> - SPDX-License-Identifier: GPL-2.0+
> - Source code comments above refering items
> - Error check for serdev_device_write's
> - Change if (xxx == NULL) to if (!xxx)
> - Remove device name from a dev_err
> - move pn533_register in _probe a bit towards the end of _probe
> - make use of newly added dev_up / dev_down phy_ops
> - control send_wakeup variable from dev_up / dev_down
> 
> Changes in v3:
> - depend on SERIAL_DEV_BUS in Kconfig
> 
> Changes in v2:
> - switched from tty line discipline to serdev, resulting in many
>   simplifications
> - SPDX License Identifier
> 
>  drivers/nfc/pn533/Kconfig  |  11 ++
>  drivers/nfc/pn533/Makefile |   2 +
>  drivers/nfc/pn533/pn533.h  |   8 +
>  drivers/nfc/pn533/uart.c   | 311 +
>  4 files changed, 332 insertions(+)
>  create mode 100644 drivers/nfc/pn533/uart.c
> 
> +static void pn532_dev_up(struct pn533 *dev)
> +{
> + struct pn532_uart_phy *pn532 = dev->phy;
> +
> + serdev_device_open(pn532->serdev);
> + pn532->send_wakeup = PN532_SEND_LAST_WAKEUP;
> +}
> +
> +static void pn532_dev_down(struct pn533 *dev)
> +{
> + struct pn532_uart_phy *pn532 = dev->phy;
> +
> + serdev_device_close(pn532->serdev);
> + pn532->send_wakeup = PN532_SEND_WAKEUP;
> +}
> +
> +static struct pn533_phy_ops uart_phy_ops = {
> + .send_frame = pn532_uart_send_frame,
> + .send_ack = pn532_uart_send_ack,
> + .abort_cmd = pn532_uart_abort_cmd,
> + .dev_up = pn532_dev_up,
> + .dev_down = pn532_dev_down,
> +};

> +static int pn532_uart_probe(struct serdev_device *serdev)
> +{
> + struct pn532_uart_phy *pn532;
> + struct pn533 *priv;
> + int err;
> +
> + err = -ENOMEM;
> + pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL);
> + if (!pn532)
> + goto err_exit;
> +
> + pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> + if (!pn532->recv_skb)
> + goto err_free;
> +
> + pn532->serdev = serdev;
> + serdev_device_set_drvdata(serdev, pn532);
> + serdev_device_set_client_ops(serdev, &pn532_serdev_ops);
> + err = serdev_device_open(serdev);
> + if (err) {
> + dev_err(&serdev->dev, "Unable to open device\n");
> + goto err_skb;
> + }
> +
> + err = serdev_device_set_baudrate(serdev, 115200);
> + if (err != 115200) {
> + err = -EINVAL;
> + goto err_serdev;
> + }
> +
> + serdev_device_set_flow_control(serdev, false);
> + pn532->send_wakeup = PN532_SEND_WAKEUP;
> + timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
> + priv = pn533_register_device(PN533_DEVICE_PN532,
> +  PN533_NO_TYPE_B_PROTOCOLS,
> +  PN533_PROTO_REQ_ACK_RESP,
> +  pn532, &uart_phy_ops, NULL,
> +  &pn532->serdev->dev,
> +  &serdev->dev);
> + if (IS_ERR(priv)) {
> + err = PTR_ERR(priv);
> + goto err_serdev;
> + }
> +
> + pn532->priv = priv;
> + err = pn533_finalize_setup(pn532->priv);
> + if (err)
> + goto err_unregister;
> +
> + serdev_device_close(serdev);

This looks broken; what if the NFC interface is brought up before this
point? You'd get a double open, which is likely to crash things, but
even if you survive that, the port would not be closed despite the
interface being up.

Can't you finalise your setup before registering the interface?

> + return 0;
> +
> +err_unregister:
> + pn533_unregister_device(pn532->priv);
> +err_serdev:
> + serdev_device_close(serdev);
> +err_skb:
> + kfree_skb(pn532->recv_skb);
> +err_free:
> + kfree(pn532);
> +err_exit:
> + return err;
> +}
> +
> +static void pn532_uart_remove(struct serdev_device *serdev)
> +{
> + struct pn532_uart_phy *pn532 = serdev_device_get_drvdata(serdev);
> +
> + pn533_unregister_device(pn532->priv);
> + serdev_device_close(serdev);

This is also broken; the port should have been closed when the interface
was deregistered.

> + kfree_skb(pn532->recv_skb);
> + kfree(pn532);
> +}

Johan


Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

2018-10-29 Thread Johan Hovold
On Mon, Oct 29, 2018 at 04:51:10PM +0100, Lars Poeschel wrote:
> On Mon, Oct 29, 2018 at 12:07:04PM +0100, Johan Hovold wrote:

> > > Wouldn't that be the scope of another later patch then ?
> > 
> > Possibly. We have accepted some serdev drivers already taking the lazy
> > approach of opening the port in probe. Depending on the driver, it may
> > not be too bad (e.g. for some specific hardware which you know you'll
> > always use), but it not really nice to have everyone pay a price in
> > terms of power-consumption for a feature that is rarely used.
> 
> Is there a way in serdev to close a port, but still occupy it ?
> I'd like to do the basic chip initialisation in _probe and then close
> the port for power-consuption reasons. I'd like to have the port still
> occupied, so that it's not available to other possible users in the
> meantime. I'd then do a serdev open again in dev_up and really use it
> from there.
> dev_down is then for serdev close and also still occupy it.
> closing and releasing would then be done in _remove.

The serdev device is bound you driver regardless of whether you open the
port or not, so just use serdev_device_open() and serdev_device_close()
as necessary at probe() if you need to do some setup and then later at
dev_up() and dev_down(), respectively.

Johan


Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

2018-10-29 Thread Johan Hovold
On Mon, Oct 29, 2018 at 11:02:46AM +0100, Lars Poeschel wrote:
> Hi Johan,
> 
> thank you very much for the review!
> 
> On Sun, Oct 28, 2018 at 11:27:25AM +0100, Johan Hovold wrote:
> > On Thu, Oct 25, 2018 at 03:29:34PM +0200, Lars Poeschel wrote:
> > > This adds the UART phy interface for the pn533 driver.
> > > The pn533 driver can be used through UART interface this way.
> > > It is implemented as a serdev device.
> > 
> > Just a few drive-by comments below.

> > > +/*
> > > + * Driver for NXP PN532 NFC Chip - UART transport layer
> > > + *
> > > + * Copyright (C) 2018 Lemonage Software GmbH
> > > + * Author: Lars Pöschel 
> > > + * All rights reserved.
> > > + */
> > 
> > > +#define VERSION "0.1"
> > 
> > We don't version kernel drivers individually, so please drop this here
> > and below.
> 
> There was a comment from Marcel about this as well and I read it as: You
> can do it, but it is not required and nobody really cares.
> I included this, because the other pn532 phy driver (i2c) is doing it
> this way, but I don't like it either, so I will drop this, as well as
> the PN532_UART_DRIVER_NAME define in the next version.

Sounds good.

> > > +static int pn532_uart_probe(struct serdev_device *serdev)
> > > +{
> > > + struct pn532_uart_phy *pn532;
> > > + struct pn533 *priv;
> > > + int err;
> > > +
> > > + err = -ENOMEM;
> > > + pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL);
> > > + if (pn532 == NULL)
> > 
> > I'd use !pn532 here (and elsewhere).
> 
> I will change it.
> 
> > > + goto err_exit;
> > > +
> > > + pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> > > + if (pn532->recv_skb == NULL)
> > > + goto err_free;
> > > +
> > > + pn532->serdev = serdev;
> > > + priv = pn533_register_device(PN533_DEVICE_PN532,
> > > +  PN533_NO_TYPE_B_PROTOCOLS,
> > > +  PN533_PROTO_REQ_ACK_RESP,
> > > +  pn532, &uart_phy_ops, NULL,
> > > +  &pn532->serdev->dev,
> > > +  &serdev->dev);
> > > +
> > 
> > Stray new line.
> 
> Ok.
> 
> > > + if (IS_ERR(priv)) {
> > > + err = PTR_ERR(priv);
> > > + goto err_skb;
> > > + }
> > 
> > Should you not set up your device fully before registering it? I'd
> > assume you could get callbacks from NFC core here.
> 
> I did not see any during my tests, but you are right: It feels a bit
> odd.
> The i2c driver is also requesting irqs after registering.
> The pn533_finalize_setup() has to be last.
> I could do the serdev_* stuff before, but ...
> 
> > > +
> > > + pn532->priv = priv;
> > > + serdev_device_set_drvdata(serdev, pn532);
> > > + serdev_device_set_client_ops(serdev, &pn532_serdev_ops);
> > > + err = serdev_device_open(serdev);
> > > + if (err) {
> > > + dev_err(&serdev->dev, "Unable to open device %s\n",
> > > + serdev->dev.init_name);
> > 
> > dev_err will include the device name so you can drop the init_name bit.
> 
> Ok, i drop it.
> 
> > > + goto err_unregister;
> > > + }
> > 
> > Keeping the serial device open at all times will prevent low power
> > states on some platforms. Wouldn't it be possible to open the device
> > when the nfc interface is brought up (and during setup)?
> 
> ... that would then be contrary to this idea.

Not necessarily, that depends on what callbacks you can expect and at
what time.

> Also I don't see how to implement it with what is there today. i2c also
> does not do something similar.

But i2c doesn't have the concept of an "open" port consuming power.

> It can be done with adding some callbacks from the core (pn533.c) driver
> to it's phy drivers.

Haven't looked at it in any detail, but in general serdev driver should
only keep the port open while the device is in use.

I only noticed that nfc core has dev_up/down callbacks which looks like
they could be used for something like this.

> Wouldn't that be the scope of another later patch then ?

Possibly. We have accepted some serdev drivers already taking the lazy
approach of opening the port in probe. Depending on the driver, it may
not be too bad (e.g. for some specific hardware which you know you'll
always use), but it not really nice to have everyone pay a price in
terms of power-consumption for a feature that is rarely used.

Johan


Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

2018-10-28 Thread Johan Hovold
On Sun, Oct 28, 2018 at 02:46:24PM +0100, Marcel Holtmann wrote:

> >> +#define VERSION "0.1"
> > 
> > We don't version kernel drivers individually, so please drop this here
> > and below.
> 
> if we don’t then maybe send patches to remove MODULE_VERSION first.
> Otherwise this is totally fine to do.
> 
> There are currently 670 usages of MODULE_VERSION and I have not heard
> anybody looking at removing this.

Should have phrased that differently; versioning modules individually
does not make much sense when we already have a kernel version which is
tied to the driver code in question and which *does* get updated as new
kernels are released (unlike these module version defines, which tend to
stay unchanged).

For USB, we've dropped module versioning entirely and push back whenever
someone proposes to add it back. Other subsystems and particularly old
drivers may still have these macros of course.

In this case, the pn533 driver (and its interface drivers) is one of only
three NFC drivers which have MODULE_VERSION, but maybe it makes sense to
keep it in, if only for consistency with the other pn533 components.

Thanks,
Johan


Re: [PATCH v3 3/5] nfc: pn533: add UART phy driver

2018-10-28 Thread Johan Hovold
On Thu, Oct 25, 2018 at 03:29:34PM +0200, Lars Poeschel wrote:
> This adds the UART phy interface for the pn533 driver.
> The pn533 driver can be used through UART interface this way.
> It is implemented as a serdev device.

Just a few drive-by comments below.

> +// SPDX-License-Identifier: GPL-2.0

This should match MODULE_LICENSE below which currently says v2 *or
later*.

> +/*
> + * Driver for NXP PN532 NFC Chip - UART transport layer
> + *
> + * Copyright (C) 2018 Lemonage Software GmbH
> + * Author: Lars Pöschel 
> + * All rights reserved.
> + */

> +#define VERSION "0.1"

We don't version kernel drivers individually, so please drop this here
and below.

> +static int pn532_uart_send_frame(struct pn533 *dev,
> + struct sk_buff *out)
> +{
> + static const u8 wakeup[] = {
> + 0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> + /* wakeup sequence and dummy bytes for waiting time */

Comments should go above the code they apply to.

> + struct pn532_uart_phy *pn532 = dev->phy;
> + int count;
> +
> + print_hex_dump_debug("PN532_uart TX: ", DUMP_PREFIX_NONE, 16, 1,
> +  out->data, out->len, false);
> +
> + pn532->cur_out_buf = out;
> + if (pn532->send_wakeup)
> + count = serdev_device_write(pn532->serdev,
> + wakeup, sizeof(wakeup),
> + MAX_SCHEDULE_TIMEOUT);

No error handling?

> +
> + count = serdev_device_write(pn532->serdev, out->data, out->len,
> + MAX_SCHEDULE_TIMEOUT);

Same here.

> + if (PN533_FRAME_CMD(((struct pn533_std_frame *)out->data)) ==
> + PN533_CMD_SAM_CONFIGURATION)
> + pn532->send_wakeup = 0;
> +
> + mod_timer(&pn532->cmd_timeout, HZ / 40 + jiffies);
> + return 0;
> +}
> +
> +static int pn532_uart_send_ack(struct pn533 *dev, gfp_t flags)
> +{
> + struct pn532_uart_phy *pn532 = dev->phy;
> + static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = {
> + 0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
> + /* spec 7.1.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */

Same as above.

> + int rc;
> +
> + rc = serdev_device_write(pn532->serdev, ack, sizeof(ack),
> + MAX_SCHEDULE_TIMEOUT);

Error handling.

> +
> + return 0;
> +}

> +static int pn532_uart_probe(struct serdev_device *serdev)
> +{
> + struct pn532_uart_phy *pn532;
> + struct pn533 *priv;
> + int err;
> +
> + err = -ENOMEM;
> + pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL);
> + if (pn532 == NULL)

I'd use !pn532 here (and elsewhere).

> + goto err_exit;
> +
> + pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
> + if (pn532->recv_skb == NULL)
> + goto err_free;
> +
> + pn532->serdev = serdev;
> + priv = pn533_register_device(PN533_DEVICE_PN532,
> +  PN533_NO_TYPE_B_PROTOCOLS,
> +  PN533_PROTO_REQ_ACK_RESP,
> +  pn532, &uart_phy_ops, NULL,
> +  &pn532->serdev->dev,
> +  &serdev->dev);
> +

Stray new line.

> + if (IS_ERR(priv)) {
> + err = PTR_ERR(priv);
> + goto err_skb;
> + }

Should you not set up your device fully before registering it? I'd
assume you could get callbacks from NFC core here.

> +
> + pn532->priv = priv;
> + serdev_device_set_drvdata(serdev, pn532);
> + serdev_device_set_client_ops(serdev, &pn532_serdev_ops);
> + err = serdev_device_open(serdev);
> + if (err) {
> + dev_err(&serdev->dev, "Unable to open device %s\n",
> + serdev->dev.init_name);

dev_err will include the device name so you can drop the init_name bit.

> + goto err_unregister;
> + }

Keeping the serial device open at all times will prevent low power
states on some platforms. Wouldn't it be possible to open the device
when the nfc interface is brought up (and during setup)?

> +
> + err = serdev_device_set_baudrate(serdev, 115200);
> + if (err != 115200) {
> + err = -EINVAL;
> + goto err_serdev;
> + }
> +
> + serdev_device_set_flow_control(serdev, false);
> + pn532->send_wakeup = 1;
> + timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
> + err = pn533_finalize_setup(pn532->priv);
> + if (err)
> + goto err_serdev;
> +
> + return 0;
> +
> +err_serdev:
> + serdev_device_close(serdev);
> +err_unregister:
> + pn533_unregister_device(pn532->priv);
> +err_skb:
> + kfree_skb(pn532->recv_skb);
> +err_free:
> + kfree(pn532);
> +err_exit:
> + return err;
> +}

> +MODULE_AUTHOR("Lars Pöschel ");
> +MODULE_DESCRIPTION("PN532 UART driver ver " VERSION);
> +MODULE_VERSION(VERSION);

Drop vers

Re: [PATCH v2 1/4] nfc: pn533: add UART phy driver

2018-10-20 Thread Johan Hovold
On Fri, Oct 19, 2018 at 10:57:28AM +0200, Lars Poeschel wrote:
> On Thu, Oct 18, 2018 at 05:00:28PM +0200, Marcel Holtmann wrote:

> > > --- a/drivers/nfc/pn533/Kconfig
> > > +++ b/drivers/nfc/pn533/Kconfig
> > > @@ -25,3 +25,13 @@ config NFC_PN533_I2C
> > > 
> > > If you choose to build a module, it'll be called pn533_i2c.
> > > Say N if unsure.
> > > +
> > > +config NFC_PN532_UART
> > > + tristate "NFC PN532 device support (UART)”
> > 
> > you are missing the "depends on SERIAL_DEV_BUS” here.
> 
> Yes, absolutely right. I missed that. I will post a follow-up.
> 
> BTW a question:
> Only enabling SERIAL_DEV_BUS did not suffice for me. I also had to
> enable SERIAL_DEV_CTRL_TTYPORT, otherwise the probe of the driver was
> not called. This seems a bit odd to me. This option seems unrelated, but
> without it, it did not work.
> 
> Should I better depend on SERIAL_DEV_CTRL_TTYPORT then ?

No, SERIAL_DEV_BUS is the correct (build-time) dependency.

In principle, your driver will work with any serdev-controller
implementation even if we currently only have one of those in the kernel
(and SERIAL_DEV_CTRL_TTYPORT therefore default to Y whenever
SERIAL_DEV_BUS is selected).

Johan


Re: [PATCH v4] NFC: pn533: don't send USB data off of the stack

2018-05-21 Thread Johan Hovold
On Sun, May 20, 2018 at 03:19:46PM +0200, Greg Kroah-Hartman wrote:
> It's amazing that this driver ever worked, but now that x86 doesn't
> allow USB data to be sent off of the stack, it really does not work at
> all.  Fix this up by properly allocating the data for the small
> "commands" that get sent to the device off of the stack.
> 
> We do this for one command by having a whole urb just for ack messages,
> as they can be submitted in interrupt context, so we can not use
> usb_bulk_msg().  But the poweron command can sleep (and does), so use
> usb_bulk_msg() for that transfer.
> 
> Reported-by: Carlos Manuel Santos 
> Cc: Samuel Ortiz 
> Cc: Stephen Hemminger 
> Cc: stable 
> Signed-off-by: Greg Kroah-Hartman 
> ---
> v4: don't use urb transfer buffer flags as the memory is tied to the urb
> (thanks to Johan)  Now we have a new static urb, and we use
> usb_bulk_msg() for the other message.
> v3: actually use the correct buffer (thanks to Arend van Spriel)
> use kmemdup (thanks to Johannes Berg and Julia Lawall)
> v2: set the urb flags correctly

Your changes look correct now so feel free to add:

Reviewed-by: Johan Hovold 

It seems we could end up returning an errno from probe with active urbs
(if pn533_finalize_setup() fails) in which case the ack buffer would
leak. But freeing the urbs while active would then be the bigger
problem, and that wasn't introduced by this patch.

Johan


Re: [PATCH v3] NFC: pn533: don't send USB data off of the stack

2018-05-18 Thread Johan Hovold
On Fri, May 18, 2018 at 12:38:11PM +0200, Greg Kroah-Hartman wrote:
> It's amazing that this driver ever worked, but now that x86 doesn't
> allow USB data to be sent off of the stack, it really does not work at
> all.  Fix this up by properly allocating the data for the small
> "commands" that get sent to the device.
> 
> The USB stack will free the buffer when the data has been transmitted,
> that is why there is no kfree() to mirror the call to kmalloc().

It looks like you're now leaking all but the final transfer buffer that
is allocated for outgoing commands, as the URBs themselves are not freed
until disconnect() (and that's when core would free the buffers along
with the URBs if URB_FREE_BUFFER is set).

Johan


[PATCH] net: rfkill: gpio: fix memory leak in probe error path

2018-04-26 Thread Johan Hovold
Make sure to free the rfkill device in case registration fails during
probe.

Fixes: 5e7ca3937fbe ("net: rfkill: gpio: convert to resource managed 
allocation")
Cc: stable  # 3.13
Cc: Heikki Krogerus 
Signed-off-by: Johan Hovold 
---
 net/rfkill/rfkill-gpio.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 41bd496531d4..00192a996be0 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -137,13 +137,18 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 
ret = rfkill_register(rfkill->rfkill_dev);
if (ret < 0)
-   return ret;
+   goto err_destroy;
 
platform_set_drvdata(pdev, rfkill);
 
dev_info(&pdev->dev, "%s device registered.\n", rfkill->name);
 
return 0;
+
+err_destroy:
+   rfkill_destroy(rfkill->rfkill_dev);
+
+   return ret;
 }
 
 static int rfkill_gpio_remove(struct platform_device *pdev)
-- 
2.17.0



Re: [PATCH] wcn36xx: fix iris child-node lookup

2017-11-13 Thread Johan Hovold
On Mon, Nov 13, 2017 at 08:51:49AM +, Kalle Valo wrote:
> Johan Hovold  writes:
> 
> > Fix child-node lookup during probe, which ended up searching the whole
> > device tree depth-first starting at the parent rather than just matching
> > on its children.
> >
> > To make things worse, the parent mmio node was also prematurely freed.
> >
> > Fixes: fd52bdae9ab0 ("wcn36xx: Disable 5GHz for wcn3620")
> 
> fd52bdae9ab0 is in net-next right now so the first release for that
> commit should be v4.15-rc1.
> 
> > Cc: stable  # 4.14
> 
> As fd52bdae9ab0 is not in v4.14 no need to Cc stable. I'll remove it.

Ah, sorry. Thanks for fixing that up.

> > Cc: Loic Poulain 
> > Signed-off-by: Johan Hovold 
> 
> Thanks, I'll queue this to v4.15.

Johan


[PATCH] wcn36xx: fix iris child-node lookup

2017-11-11 Thread Johan Hovold
Fix child-node lookup during probe, which ended up searching the whole
device tree depth-first starting at the parent rather than just matching
on its children.

To make things worse, the parent mmio node was also prematurely freed.

Fixes: fd52bdae9ab0 ("wcn36xx: Disable 5GHz for wcn3620")
Cc: stable  # 4.14
Cc: Loic Poulain 
Signed-off-by: Johan Hovold 
---
 drivers/net/wireless/ath/wcn36xx/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c 
b/drivers/net/wireless/ath/wcn36xx/main.c
index 71812a2dd513..f7d228b5ba93 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1233,7 +1233,7 @@ static int wcn36xx_platform_get_resources(struct wcn36xx 
*wcn,
}
 
/* External RF module */
-   iris_node = of_find_node_by_name(mmio_node, "iris");
+   iris_node = of_get_child_by_name(mmio_node, "iris");
if (iris_node) {
if (of_device_is_compatible(iris_node, "qcom,wcn3620"))
wcn->rf_id = RF_IRIS_WCN3620;
-- 
2.15.0



Re: [PATCH] NFC: fix device-allocation error return

2017-08-28 Thread Johan Hovold
Samuel or David,

On Sat, Jul 22, 2017 at 03:32:28PM +0200, Johan Hovold wrote:
> On Sun, Jul 09, 2017 at 01:08:58PM +0200, Johan Hovold wrote:
> > A recent change fixing NFC device allocation itself introduced an
> > error-handling bug by returning an error pointer in case device-id
> > allocation failed. This is clearly broken as the callers still expected
> > NULL to be returned on errors as detected by Dan's static checker.
> > 
> > Fix this up by returning NULL in the event that we've run out of memory
> > when allocating a new device id.
> > 
> > Note that the offending commit is marked for stable (3.8) so this fix
> > needs to be backported along with it.
> > 
> > Fixes: 20777bc57c34 ("NFC: fix broken device allocation")
> > Cc: stable  # 3.8
> > Reported-by: Dan Carpenter 
> > Signed-off-by: Johan Hovold 

> Could you apply this follow-up fix so that it can be backported along
> with the offending commit (which was just added to the stable queues)?
> 
> We would only hit this error path if an ida allocation fails due to OOM;
> so while this is not critical, it would still be nice to get it fixed.

Another reminder about this one; can you apply it so we can get it into
4.14-rc1?

Note that the offending commit has now been backported to the stable
trees and we really want this trivial follow-up fix to be backported as
well.

Let me know if you want me to resend the patch.

Thanks,
Johan


Re: [PATCH] NFC: fix device-allocation error return

2017-07-22 Thread Johan Hovold
On Sun, Jul 09, 2017 at 01:08:58PM +0200, Johan Hovold wrote:
> A recent change fixing NFC device allocation itself introduced an
> error-handling bug by returning an error pointer in case device-id
> allocation failed. This is clearly broken as the callers still expected
> NULL to be returned on errors as detected by Dan's static checker.
> 
> Fix this up by returning NULL in the event that we've run out of memory
> when allocating a new device id.
> 
> Note that the offending commit is marked for stable (3.8) so this fix
> needs to be backported along with it.
> 
> Fixes: 20777bc57c34 ("NFC: fix broken device allocation")
> Cc: stable    # 3.8
> Reported-by: Dan Carpenter 
> Signed-off-by: Johan Hovold 

Samuel or David,

Could you apply this follow-up fix so that it can be backported along
with the offending commit (which was just added to the stable queues)?

We would only hit this error path if an ida allocation fails due to OOM;
so while this is not critical, it would still be nice to get it fixed.

Thanks,
Johan


[PATCH] NFC: fix device-allocation error return

2017-07-09 Thread Johan Hovold
A recent change fixing NFC device allocation itself introduced an
error-handling bug by returning an error pointer in case device-id
allocation failed. This is clearly broken as the callers still expected
NULL to be returned on errors as detected by Dan's static checker.

Fix this up by returning NULL in the event that we've run out of memory
when allocating a new device id.

Note that the offending commit is marked for stable (3.8) so this fix
needs to be backported along with it.

Fixes: 20777bc57c34 ("NFC: fix broken device allocation")
Cc: stable  # 3.8
Reported-by: Dan Carpenter 
Signed-off-by: Johan Hovold 
---
 net/nfc/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/nfc/core.c b/net/nfc/core.c
index 5cf33df888c3..c699d64a0753 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -1106,7 +1106,7 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
 err_free_dev:
kfree(dev);
 
-   return ERR_PTR(rc);
+   return NULL;
 }
 EXPORT_SYMBOL(nfc_allocate_device);
 
-- 
2.13.2



Re: [bug report] NFC: fix broken device allocation

2017-07-07 Thread Johan Hovold
On Fri, Jul 07, 2017 at 12:33:34PM +0300, Dan Carpenter wrote:
> Hello Johan Hovold,
> 
> The patch 20777bc57c34: "NFC: fix broken device allocation" from Mar
> 30, 2017, leads to the following static checker warning:
> 
>   drivers/nfc/pn533/pn533.c:2653 pn533_register_device()
>   error: 'priv->nfc_dev' dereferencing possible ERR_PTR()

> drivers/nfc/pn533/pn533.c
>   2639  skb_queue_head_init(&priv->resp_q);
>   2640  skb_queue_head_init(&priv->fragment_skb);
>   2641  
>   2642  INIT_LIST_HEAD(&priv->cmd_queue);
>   2643  
>   2644  priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
>   2645 priv->ops->tx_header_len +
>   2646 
> PN533_CMD_DATAEXCH_HEAD_LEN,
>   2647 priv->ops->tx_tail_len);
> 
> We changed this to return error pointers as well as NULL.  When
> functions return a NULL as well as error pointers, then NULL is supposed
> to be a special case of success but here it's just a failure.  That's
> messy and bug prone.

Thanks for reporting this, Dan.

I'll take a closer look at this tomorrow, but I guess we could just
continue using NULL for all errors for now.

Thanks,
Johan


Re: [PATCH v2 0/8] NFC: fix device allocation and nfcmrvl crashes

2017-06-01 Thread Johan Hovold
Hi Samuel,

On Tue, May 16, 2017 at 11:42:29AM +0200, Johan Hovold wrote:

> On Thu, Apr 27, 2017 at 12:42:38AM +0200, Samuel Ortiz wrote:
> > Hi Johan,
> > 
> > On Thu, Mar 30, 2017 at 12:15:34PM +0200, Johan Hovold wrote:
> > > This started out with the observation that the nfcmrvl_uart driver
> > > unconditionally dereferenced the tty class device despite the fact that
> > > not every tty has an associated struct device (Unix98 ptys). Some
> > > further changes were needed in the common nfcmrvl code to fully address
> > > this, some of which also incidentally fixed a few related bugs (e.g.
> > > resource leaks in error paths).
> > > 
> > > While fixing this I stumbled over a regression in NFC core that lead to
> > > broken registration error paths and misnamed workqueues.
> > > 
> > > Note that this has only been tested by configuring the n_hci line
> > > discipline for different ttys without any actual NFC hardware connected.
> > > 
> > > Johan
> > > 
> > > 
> > > Changes in v2
> > >  - fix typo in commit message (1/8)
> > >  - release reset gpio in error paths (3/8)
> > >  - fix description of patch impact (3/8)
> > >  - allow gpio 0 to be used for reset signalling (8/8, new)
> > > 
> > > 
> > > Johan Hovold (8):
> > >   NFC: fix broken device allocation
> > >   NFC: nfcmrvl_uart: add missing tty-device sanity check
> > >   NFC: nfcmrvl: do not use device-managed resources
> > >   NFC: nfcmrvl: use nfc-device for firmware download
> > >   NFC: nfcmrvl: fix firmware-management initialisation
> > >   NFC: nfcmrvl_uart: fix device-node leak during probe
> > >   NFC: nfcmrvl_usb: use interface as phy device
> > >   NFC: nfcmrvl: allow gpio 0 for reset signalling
> > Applied, thanks.
> 
> These never made it into net-next and 4.12-rc1, so will you be sending
> them on as fixes for 4.12-rc instead?

Thought I'd send another reminder: will you forward these fixes (that
are still in your nfc-next tree) for 4.12?

As this series fix a crash that can be triggered by an unprivileged
user, I really think the first five patches at least need to go into
4.12.

Thanks,
Johan


Re: [PATCH v2 0/8] NFC: fix device allocation and nfcmrvl crashes

2017-05-16 Thread Johan Hovold
Hi Samuel,

On Thu, Apr 27, 2017 at 12:42:38AM +0200, Samuel Ortiz wrote:
> Hi Johan,
> 
> On Thu, Mar 30, 2017 at 12:15:34PM +0200, Johan Hovold wrote:
> > This started out with the observation that the nfcmrvl_uart driver
> > unconditionally dereferenced the tty class device despite the fact that
> > not every tty has an associated struct device (Unix98 ptys). Some
> > further changes were needed in the common nfcmrvl code to fully address
> > this, some of which also incidentally fixed a few related bugs (e.g.
> > resource leaks in error paths).
> > 
> > While fixing this I stumbled over a regression in NFC core that lead to
> > broken registration error paths and misnamed workqueues.
> > 
> > Note that this has only been tested by configuring the n_hci line
> > discipline for different ttys without any actual NFC hardware connected.
> > 
> > Johan
> > 
> > 
> > Changes in v2
> >  - fix typo in commit message (1/8)
> >  - release reset gpio in error paths (3/8)
> >  - fix description of patch impact (3/8)
> >  - allow gpio 0 to be used for reset signalling (8/8, new)
> > 
> > 
> > Johan Hovold (8):
> >   NFC: fix broken device allocation
> >   NFC: nfcmrvl_uart: add missing tty-device sanity check
> >   NFC: nfcmrvl: do not use device-managed resources
> >   NFC: nfcmrvl: use nfc-device for firmware download
> >   NFC: nfcmrvl: fix firmware-management initialisation
> >   NFC: nfcmrvl_uart: fix device-node leak during probe
> >   NFC: nfcmrvl_usb: use interface as phy device
> >   NFC: nfcmrvl: allow gpio 0 for reset signalling
> Applied, thanks.

These never made it into net-next and 4.12-rc1, so will you be sending
them on as fixes for 4.12-rc instead?

Thanks,
Johan


[PATCH] wireless: mwifiex: add missing USB-descriptor endianness conversion

2017-05-12 Thread Johan Hovold
Add the missing endianness conversions to a debug statement printing
the USB device-descriptor bcdUSB field during probe.

Signed-off-by: Johan Hovold 
---
 drivers/net/wireless/marvell/mwifiex/usb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c 
b/drivers/net/wireless/marvell/mwifiex/usb.c
index 2f7705c50161..debd6216366a 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -424,7 +424,8 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
card->intf = intf;
 
pr_debug("info: bcdUSB=%#x Device Class=%#x SubClass=%#x 
Protocol=%#x\n",
-udev->descriptor.bcdUSB, udev->descriptor.bDeviceClass,
+le16_to_cpu(udev->descriptor.bcdUSB),
+udev->descriptor.bDeviceClass,
 udev->descriptor.bDeviceSubClass,
 udev->descriptor.bDeviceProtocol);
 
-- 
2.13.0



Re: [PATCH v2 0/8] NFC: fix device allocation and nfcmrvl crashes

2017-04-26 Thread Johan Hovold
On Wed, Apr 19, 2017 at 01:24:31AM +0200, Samuel Ortiz wrote:

> On Tue, Apr 18, 2017 at 12:09:16PM +0200, Johan Hovold wrote:
> > On Thu, Mar 30, 2017 at 12:15:34PM +0200, Johan Hovold wrote:
> > > This started out with the observation that the nfcmrvl_uart driver
> > > unconditionally dereferenced the tty class device despite the fact that
> > > not every tty has an associated struct device (Unix98 ptys). Some
> > > further changes were needed in the common nfcmrvl code to fully address
> > > this, some of which also incidentally fixed a few related bugs (e.g.
> > > resource leaks in error paths).
> > > 
> > > While fixing this I stumbled over a regression in NFC core that lead to
> > > broken registration error paths and misnamed workqueues.
> > > 
> > > Note that this has only been tested by configuring the n_hci line
> > > discipline for different ttys without any actual NFC hardware connected.
> > 
> > > Johan Hovold (8):
> > >   NFC: fix broken device allocation
> > >   NFC: nfcmrvl_uart: add missing tty-device sanity check
> > >   NFC: nfcmrvl: do not use device-managed resources
> > >   NFC: nfcmrvl: use nfc-device for firmware download
> > >   NFC: nfcmrvl: fix firmware-management initialisation
> > >   NFC: nfcmrvl_uart: fix device-node leak during probe
> > >   NFC: nfcmrvl_usb: use interface as phy device
> > >   NFC: nfcmrvl: allow gpio 0 for reset signalling
> > 
> > Any chance of getting these into 4.12, Samuel?

> I have yours and Mark Greer patchset pending. I'll push them to nfc-next
> and see if I can send another pull request to Dave by the end of this
> week.

Any progress on this now that we got another week?

Thanks,
Johan


Re: [PATCH v2 0/8] NFC: fix device allocation and nfcmrvl crashes

2017-04-18 Thread Johan Hovold
On Thu, Mar 30, 2017 at 12:15:34PM +0200, Johan Hovold wrote:
> This started out with the observation that the nfcmrvl_uart driver
> unconditionally dereferenced the tty class device despite the fact that
> not every tty has an associated struct device (Unix98 ptys). Some
> further changes were needed in the common nfcmrvl code to fully address
> this, some of which also incidentally fixed a few related bugs (e.g.
> resource leaks in error paths).
> 
> While fixing this I stumbled over a regression in NFC core that lead to
> broken registration error paths and misnamed workqueues.
> 
> Note that this has only been tested by configuring the n_hci line
> discipline for different ttys without any actual NFC hardware connected.

> Johan Hovold (8):
>   NFC: fix broken device allocation
>   NFC: nfcmrvl_uart: add missing tty-device sanity check
>   NFC: nfcmrvl: do not use device-managed resources
>   NFC: nfcmrvl: use nfc-device for firmware download
>   NFC: nfcmrvl: fix firmware-management initialisation
>   NFC: nfcmrvl_uart: fix device-node leak during probe
>   NFC: nfcmrvl_usb: use interface as phy device
>   NFC: nfcmrvl: allow gpio 0 for reset signalling

Any chance of getting these into 4.12, Samuel?

Note that patches 2 and 4 fixes NULL-derefs that can be triggered by an
unprivileged user.

Thanks,
Johan


Re: [PATCH 1/2] wireless: ath9k_htc: fix NULL-deref at probe

2017-04-03 Thread Johan Hovold
On Mon, Apr 03, 2017 at 01:21:08PM +, Kalle Valo wrote:
> Johan Hovold  writes:
> 
> > On Mon, Apr 03, 2017 at 01:02:28PM +, Kalle Valo wrote:
> >> Kalle Valo  writes:
> >> 
> >> > Johan Hovold  writes:
> >> >
> >> >> On Mon, Mar 13, 2017 at 01:44:20PM +0100, Johan Hovold wrote:
> >> >>> Make sure to check the number of endpoints to avoid dereferencing a
> >> >>> NULL-pointer or accessing memory beyond the endpoint array should a
> >> >>> malicious device lack the expected endpoints.
> >> >>> 
> >> >>> Fixes: 36bcce430657 ("ath9k_htc: Handle storage devices")
> >> >>> Cc: Sujith Manoharan 
> >> >>> Signed-off-by: Johan Hovold 
> >> >>
> >> >> Is this one still in your queue, Kalle?
> >> >
> >> > Yes, I'm just lacking behing:
> >> >
> >> > https://patchwork.kernel.org/patch/9620723/
> >> 
> >> Meant "lagging" of course. Mondays..
> >> 
> >> >> As I mentioned earlier, I should have added a
> >> >>
> >> >> Cc: stable  # 2.6.39
> >> >>
> >> >> but left it out as I mistakingly thought the net recommendations to do
> >> >> so applied also to wireless.
> >> >
> >> > Ok, I'll add that.
> >> 
> >> But is 2.6.39 really correct? Shouldn't it be 2.6.39+ so that it means
> >> all versions since 2.6.39?
> >
> > Either way is fine, the stable maintainers apply them to all later
> > versions.
> >
> > I notice now that adding a plus sign is more common, but it's still a
> > 1:2 ratio judging from quick grep, while the stable-kernel-rules.rst
> > actually uses a minus sign...
> 
> Heh, quite confusing :) I added the plus sign already to the patch in my
> pending branch so unless you object I'll keep it.

Please do, no objection. :)

Thanks,
Johan


Re: [PATCH 1/2] wireless: ath9k_htc: fix NULL-deref at probe

2017-04-03 Thread Johan Hovold
On Mon, Apr 03, 2017 at 01:02:28PM +, Kalle Valo wrote:
> Kalle Valo  writes:
> 
> > Johan Hovold  writes:
> >
> >> On Mon, Mar 13, 2017 at 01:44:20PM +0100, Johan Hovold wrote:
> >>> Make sure to check the number of endpoints to avoid dereferencing a
> >>> NULL-pointer or accessing memory beyond the endpoint array should a
> >>> malicious device lack the expected endpoints.
> >>> 
> >>> Fixes: 36bcce430657 ("ath9k_htc: Handle storage devices")
> >>> Cc: Sujith Manoharan 
> >>> Signed-off-by: Johan Hovold 
> >>
> >> Is this one still in your queue, Kalle?
> >
> > Yes, I'm just lacking behing:
> >
> > https://patchwork.kernel.org/patch/9620723/
> 
> Meant "lagging" of course. Mondays..
> 
> >> As I mentioned earlier, I should have added a
> >>
> >> Cc: stable  # 2.6.39
> >>
> >> but left it out as I mistakingly thought the net recommendations to do
> >> so applied also to wireless.
> >
> > Ok, I'll add that.
> 
> But is 2.6.39 really correct? Shouldn't it be 2.6.39+ so that it means
> all versions since 2.6.39?

Either way is fine, the stable maintainers apply them to all later
versions.

I notice now that adding a plus sign is more common, but it's still a
1:2 ratio judging from quick grep, while the stable-kernel-rules.rst
actually uses a minus sign...

Thanks,
Johan


Re: [PATCH 1/2] wireless: ath9k_htc: fix NULL-deref at probe

2017-04-03 Thread Johan Hovold
On Mon, Mar 13, 2017 at 01:44:20PM +0100, Johan Hovold wrote:
> Make sure to check the number of endpoints to avoid dereferencing a
> NULL-pointer or accessing memory beyond the endpoint array should a
> malicious device lack the expected endpoints.
> 
> Fixes: 36bcce430657 ("ath9k_htc: Handle storage devices")
> Cc: Sujith Manoharan 
> Signed-off-by: Johan Hovold 

Is this one still in your queue, Kalle?

As I mentioned earlier, I should have added a

Cc: stable  # 2.6.39

but left it out as I mistakingly thought the net recommendations to do
so applied also to wireless.

Thanks,
Johan


[PATCH v2 1/8] NFC: fix broken device allocation

2017-03-30 Thread Johan Hovold
Commit 7eda8b8e9677 ("NFC: Use IDR library to assing NFC devices IDs")
moved device-id allocation and struct-device initialisation from
nfc_allocate_device() to nfc_register_device().

This broke just about every nfc-device-registration error path, which
continue to call nfc_free_device() that tries to put the device
reference of the now uninitialised (but zeroed) struct device:

kobject: '(null)' (ce316420): is not initialized, yet kobject_put() is being 
called.

The late struct-device initialisation also meant that various work
queues whose names are derived from the nfc device name were also
misnamed:

  421 root 0 SW<  [(null)_nci_cmd_]
  422 root 0 SW<  [(null)_nci_rx_w]
  423 root 0 SW<  [(null)_nci_tx_w]

Move the id-allocation and struct-device initialisation back to
nfc_allocate_device() and fix up the single call site which did not use
nfc_free_device() in its error path.

Fixes: 7eda8b8e9677 ("NFC: Use IDR library to assing NFC devices IDs")
Cc: stable  # 3.8
Cc: Samuel Ortiz 
Signed-off-by: Johan Hovold 
---
 net/nfc/core.c | 31 ++-
 net/nfc/nci/core.c |  3 +--
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/nfc/core.c b/net/nfc/core.c
index 122bb81da918..5cf33df888c3 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -982,6 +982,8 @@ static void nfc_release(struct device *d)
kfree(se);
}
 
+   ida_simple_remove(&nfc_index_ida, dev->idx);
+
kfree(dev);
 }
 
@@ -1056,6 +1058,7 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
int tx_headroom, int tx_tailroom)
 {
struct nfc_dev *dev;
+   int rc;
 
if (!ops->start_poll || !ops->stop_poll || !ops->activate_target ||
!ops->deactivate_target || !ops->im_transceive)
@@ -1068,6 +1071,15 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
if (!dev)
return NULL;
 
+   rc = ida_simple_get(&nfc_index_ida, 0, 0, GFP_KERNEL);
+   if (rc < 0)
+   goto err_free_dev;
+   dev->idx = rc;
+
+   dev->dev.class = &nfc_class;
+   dev_set_name(&dev->dev, "nfc%d", dev->idx);
+   device_initialize(&dev->dev);
+
dev->ops = ops;
dev->supported_protocols = supported_protocols;
dev->tx_headroom = tx_headroom;
@@ -1090,6 +1102,11 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
}
 
return dev;
+
+err_free_dev:
+   kfree(dev);
+
+   return ERR_PTR(rc);
 }
 EXPORT_SYMBOL(nfc_allocate_device);
 
@@ -1104,14 +1121,6 @@ int nfc_register_device(struct nfc_dev *dev)
 
pr_debug("dev_name=%s\n", dev_name(&dev->dev));
 
-   dev->idx = ida_simple_get(&nfc_index_ida, 0, 0, GFP_KERNEL);
-   if (dev->idx < 0)
-   return dev->idx;
-
-   dev->dev.class = &nfc_class;
-   dev_set_name(&dev->dev, "nfc%d", dev->idx);
-   device_initialize(&dev->dev);
-
mutex_lock(&nfc_devlist_mutex);
nfc_devlist_generation++;
rc = device_add(&dev->dev);
@@ -1149,12 +1158,10 @@ EXPORT_SYMBOL(nfc_register_device);
  */
 void nfc_unregister_device(struct nfc_dev *dev)
 {
-   int rc, id;
+   int rc;
 
pr_debug("dev_name=%s\n", dev_name(&dev->dev));
 
-   id = dev->idx;
-
if (dev->rfkill) {
rfkill_unregister(dev->rfkill);
rfkill_destroy(dev->rfkill);
@@ -1179,8 +1186,6 @@ void nfc_unregister_device(struct nfc_dev *dev)
nfc_devlist_generation++;
device_del(&dev->dev);
mutex_unlock(&nfc_devlist_mutex);
-
-   ida_simple_remove(&nfc_index_ida, id);
 }
 EXPORT_SYMBOL(nfc_unregister_device);
 
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 61fff422424f..85a3d9ed4c29 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1173,8 +1173,7 @@ struct nci_dev *nci_allocate_device(struct nci_ops *ops,
return ndev;
 
 free_nfc:
-   kfree(ndev->nfc_dev);
-
+   nfc_free_device(ndev->nfc_dev);
 free_nci:
kfree(ndev);
return NULL;
-- 
2.12.2



[PATCH v2 0/8] NFC: fix device allocation and nfcmrvl crashes

2017-03-30 Thread Johan Hovold
This started out with the observation that the nfcmrvl_uart driver
unconditionally dereferenced the tty class device despite the fact that
not every tty has an associated struct device (Unix98 ptys). Some
further changes were needed in the common nfcmrvl code to fully address
this, some of which also incidentally fixed a few related bugs (e.g.
resource leaks in error paths).

While fixing this I stumbled over a regression in NFC core that lead to
broken registration error paths and misnamed workqueues.

Note that this has only been tested by configuring the n_hci line
discipline for different ttys without any actual NFC hardware connected.

Johan


Changes in v2
 - fix typo in commit message (1/8)
 - release reset gpio in error paths (3/8)
 - fix description of patch impact (3/8)
 - allow gpio 0 to be used for reset signalling (8/8, new)


Johan Hovold (8):
  NFC: fix broken device allocation
  NFC: nfcmrvl_uart: add missing tty-device sanity check
  NFC: nfcmrvl: do not use device-managed resources
  NFC: nfcmrvl: use nfc-device for firmware download
  NFC: nfcmrvl: fix firmware-management initialisation
  NFC: nfcmrvl_uart: fix device-node leak during probe
  NFC: nfcmrvl_usb: use interface as phy device
  NFC: nfcmrvl: allow gpio 0 for reset signalling

 drivers/nfc/nfcmrvl/fw_dnld.c |  7 --
 drivers/nfc/nfcmrvl/main.c| 40 +++
 drivers/nfc/nfcmrvl/uart.c| 11 ++
 drivers/nfc/nfcmrvl/usb.c |  4 +---
 include/linux/platform_data/nfcmrvl.h |  2 +-
 net/nfc/core.c| 31 +++
 net/nfc/nci/core.c|  3 +--
 7 files changed, 55 insertions(+), 43 deletions(-)

-- 
2.12.2



[PATCH v2 3/8] NFC: nfcmrvl: do not use device-managed resources

2017-03-30 Thread Johan Hovold
This specifically fixes resource leaks in the registration error paths.

Device-managed resources is a bad fit for this driver as devices can be
registered from the n_nci line discipline. Firstly, a tty may not even
have a corresponding device (should it be part of a Unix98 pty)
something which would lead to a NULL-pointer dereference when
registering resources.

Secondly, if the tty has a class device, its lifetime exceeds that of
the line discipline, which means that resources would leak every time
the line discipline is closed (or if registration fails).

Currently, the devres interface was only being used to request a reset
gpio despite the fact that it was already explicitly freed in
nfcmrvl_nci_unregister_dev() (along with the private data), something
which also prevented the resource leak at close.

Note that the driver treats gpio number 0 as invalid despite it being
perfectly valid. This will be addressed in a follow-up patch.

Fixes: b2fe288eac72 ("NFC: nfcmrvl: free reset gpio")
Fixes: 4a2b947f56b3 ("NFC: nfcmrvl: add chip reset management")
Cc: stable  # 4.2: b2fe288eac72
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/main.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 51c8240a1672..3e3fc9588f10 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -124,12 +124,13 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
memcpy(&priv->config, pdata, sizeof(*pdata));
 
if (priv->config.reset_n_io) {
-   rc = devm_gpio_request_one(dev,
-  priv->config.reset_n_io,
-  GPIOF_OUT_INIT_LOW,
-  "nfcmrvl_reset_n");
-   if (rc < 0)
+   rc = gpio_request_one(priv->config.reset_n_io,
+ GPIOF_OUT_INIT_LOW,
+ "nfcmrvl_reset_n");
+   if (rc < 0) {
+   priv->config.reset_n_io = 0;
nfc_err(dev, "failed to request reset_n io\n");
+   }
}
 
if (phy == NFCMRVL_PHY_SPI) {
@@ -154,7 +155,7 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
if (!priv->ndev) {
nfc_err(dev, "nci_allocate_device failed\n");
rc = -ENOMEM;
-   goto error;
+   goto error_free_gpio;
}
 
nci_set_drvdata(priv->ndev, priv);
@@ -179,7 +180,9 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
 
 error_free_dev:
nci_free_device(priv->ndev);
-error:
+error_free_gpio:
+   if (priv->config.reset_n_io)
+   gpio_free(priv->config.reset_n_io);
kfree(priv);
return ERR_PTR(rc);
 }
@@ -195,7 +198,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private 
*priv)
nfcmrvl_fw_dnld_deinit(priv);
 
if (priv->config.reset_n_io)
-   devm_gpio_free(priv->dev, priv->config.reset_n_io);
+   gpio_free(priv->config.reset_n_io);
 
nci_unregister_device(ndev);
nci_free_device(ndev);
-- 
2.12.2



[PATCH v2 2/8] NFC: nfcmrvl_uart: add missing tty-device sanity check

2017-03-30 Thread Johan Hovold
Make sure to check the tty-device pointer before trying to access the
parent device to avoid dereferencing a NULL-pointer when the tty is one
end of a Unix98 pty.

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Cc: stable  # 4.2
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/uart.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 83a99e38e7bd..6c0c301611c4 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -109,6 +109,7 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
struct nfcmrvl_private *priv;
struct nfcmrvl_platform_data *pdata = NULL;
struct nfcmrvl_platform_data config;
+   struct device *dev = nu->tty->dev;
 
/*
 * Platform data cannot be used here since usually it is already used
@@ -116,9 +117,8 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
 * and check if DT entries were added.
 */
 
-   if (nu->tty->dev->parent && nu->tty->dev->parent->of_node)
-   if (nfcmrvl_uart_parse_dt(nu->tty->dev->parent->of_node,
- &config) == 0)
+   if (dev && dev->parent && dev->parent->of_node)
+   if (nfcmrvl_uart_parse_dt(dev->parent->of_node, &config) == 0)
pdata = &config;
 
if (!pdata) {
@@ -131,7 +131,7 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
}
 
priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_UART, nu, &uart_ops,
-   nu->tty->dev, pdata);
+   dev, pdata);
if (IS_ERR(priv))
return PTR_ERR(priv);
 
-- 
2.12.2



[PATCH v2 8/8] NFC: nfcmrvl: allow gpio 0 for reset signalling

2017-03-30 Thread Johan Hovold
Allow gpio 0 to be used for reset signalling, and instead use negative
errnos to disable the reset functionality.

Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/main.c| 9 -
 include/linux/platform_data/nfcmrvl.h | 2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index a446590a71ca..838627fa82ee 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -123,12 +123,12 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
 
memcpy(&priv->config, pdata, sizeof(*pdata));
 
-   if (priv->config.reset_n_io) {
+   if (gpio_is_valid(priv->config.reset_n_io)) {
rc = gpio_request_one(priv->config.reset_n_io,
  GPIOF_OUT_INIT_LOW,
  "nfcmrvl_reset_n");
if (rc < 0) {
-   priv->config.reset_n_io = 0;
+   priv->config.reset_n_io = -EINVAL;
nfc_err(dev, "failed to request reset_n io\n");
}
}
@@ -183,7 +183,7 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
 error_free_dev:
nci_free_device(priv->ndev);
 error_free_gpio:
-   if (priv->config.reset_n_io)
+   if (gpio_is_valid(priv->config.reset_n_io))
gpio_free(priv->config.reset_n_io);
kfree(priv);
return ERR_PTR(rc);
@@ -199,7 +199,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private 
*priv)
 
nfcmrvl_fw_dnld_deinit(priv);
 
-   if (priv->config.reset_n_io)
+   if (gpio_is_valid(priv->config.reset_n_io))
gpio_free(priv->config.reset_n_io);
 
nci_unregister_device(ndev);
@@ -267,7 +267,6 @@ int nfcmrvl_parse_dt(struct device_node *node,
reset_n_io = of_get_named_gpio(node, "reset-n-io", 0);
if (reset_n_io < 0) {
pr_info("no reset-n-io config\n");
-   reset_n_io = 0;
} else if (!gpio_is_valid(reset_n_io)) {
pr_err("invalid reset-n-io GPIO\n");
return reset_n_io;
diff --git a/include/linux/platform_data/nfcmrvl.h 
b/include/linux/platform_data/nfcmrvl.h
index a6f9d633f5be..9e75ac8d19be 100644
--- a/include/linux/platform_data/nfcmrvl.h
+++ b/include/linux/platform_data/nfcmrvl.h
@@ -23,7 +23,7 @@ struct nfcmrvl_platform_data {
 */
 
/* GPIO that is wired to RESET_N signal */
-   unsigned int reset_n_io;
+   int reset_n_io;
/* Tell if transport is muxed in HCI one */
unsigned int hci_muxed;
 
-- 
2.12.2



[PATCH v2 7/8] NFC: nfcmrvl_usb: use interface as phy device

2017-03-30 Thread Johan Hovold
Use the USB-interface rather than parent USB-device device, which is
what this driver binds to, when registering the nci device.

Note that using the right device is important when dealing with device-
managed resources as the interface can be unbound independently of the
parent device.

Also note that private device pointer had already been set by
nfcmrvl_nci_register_dev() so the redundant assignment can therefore be
removed.

Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/usb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index 585a0f20835b..728b3321842d 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -341,15 +341,13 @@ static int nfcmrvl_probe(struct usb_interface *intf,
init_usb_anchor(&drv_data->deferred);
 
priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_USB, drv_data, &usb_ops,
-   &drv_data->udev->dev, &config);
+   &intf->dev, &config);
if (IS_ERR(priv))
return PTR_ERR(priv);
 
drv_data->priv = priv;
drv_data->priv->support_fw_dnld = false;
 
-   priv->dev = &drv_data->udev->dev;
-
usb_set_intfdata(intf, drv_data);
 
return 0;
-- 
2.12.2



[PATCH v2 4/8] NFC: nfcmrvl: use nfc-device for firmware download

2017-03-30 Thread Johan Hovold
Use the nfc- rather than phy-device in firmware-management code that
needs a valid struct device.

This specifically fixes a NULL-pointer dereference in
nfcmrvl_fw_dnld_init() during registration when the underlying tty is
one end of a Unix98 pty.

Note that the driver still uses the phy device for any debugging, which
is fine for now.

Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Cc: stable  # 4.4
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/fw_dnld.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
index f8dcdf4b24f6..af62c4c854f3 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -459,7 +459,7 @@ int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv)
 
INIT_WORK(&priv->fw_dnld.rx_work, fw_dnld_rx_work);
snprintf(name, sizeof(name), "%s_nfcmrvl_fw_dnld_rx_wq",
-dev_name(priv->dev));
+dev_name(&priv->ndev->nfc_dev->dev));
priv->fw_dnld.rx_wq = create_singlethread_workqueue(name);
if (!priv->fw_dnld.rx_wq)
return -ENOMEM;
@@ -496,6 +496,7 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char 
*firmware_name)
 {
struct nfcmrvl_private *priv = nci_get_drvdata(ndev);
struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+   int res;
 
if (!priv->support_fw_dnld)
return -ENOTSUPP;
@@ -511,7 +512,9 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char 
*firmware_name)
 */
 
/* Retrieve FW binary */
-   if (request_firmware(&fw_dnld->fw, firmware_name, priv->dev) < 0) {
+   res = request_firmware(&fw_dnld->fw, firmware_name,
+  &ndev->nfc_dev->dev);
+   if (res < 0) {
nfc_err(priv->dev, "failed to retrieve FW %s", firmware_name);
return -ENOENT;
}
-- 
2.12.2



[PATCH v2 5/8] NFC: nfcmrvl: fix firmware-management initialisation

2017-03-30 Thread Johan Hovold
The nci-device was never deregistered in the event that
fw-initialisation failed.

Fix this by moving the firmware initialisation before device
registration since the firmware work queue should be available before
registering.

Note that this depends on a recent fix that moved device-name
initialisation back to to nci_allocate_device() as the
firmware-workqueue name is now derived from the nfc-device name.

Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Cc: stable  # 4.4
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/main.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 3e3fc9588f10..a446590a71ca 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -158,26 +158,28 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
goto error_free_gpio;
}
 
+   rc = nfcmrvl_fw_dnld_init(priv);
+   if (rc) {
+   nfc_err(dev, "failed to initialize FW download %d\n", rc);
+   goto error_free_dev;
+   }
+
nci_set_drvdata(priv->ndev, priv);
 
rc = nci_register_device(priv->ndev);
if (rc) {
nfc_err(dev, "nci_register_device failed %d\n", rc);
-   goto error_free_dev;
+   goto error_fw_dnld_deinit;
}
 
/* Ensure that controller is powered off */
nfcmrvl_chip_halt(priv);
 
-   rc = nfcmrvl_fw_dnld_init(priv);
-   if (rc) {
-   nfc_err(dev, "failed to initialize FW download %d\n", rc);
-   goto error_free_dev;
-   }
-
nfc_info(dev, "registered with nci successfully\n");
return priv;
 
+error_fw_dnld_deinit:
+   nfcmrvl_fw_dnld_deinit(priv);
 error_free_dev:
nci_free_device(priv->ndev);
 error_free_gpio:
-- 
2.12.2



[PATCH v2 6/8] NFC: nfcmrvl_uart: fix device-node leak during probe

2017-03-30 Thread Johan Hovold
Make sure to release the device-node reference when done parsing the
node.

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/uart.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 6c0c301611c4..91162f8e0366 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -84,6 +84,7 @@ static int nfcmrvl_uart_parse_dt(struct device_node *node,
ret = nfcmrvl_parse_dt(matched_node, pdata);
if (ret < 0) {
pr_err("Failed to get generic entries\n");
+   of_node_put(matched_node);
return ret;
}
 
@@ -97,6 +98,8 @@ static int nfcmrvl_uart_parse_dt(struct device_node *node,
else
pdata->break_control = 0;
 
+   of_node_put(matched_node);
+
return 0;
 }
 
-- 
2.12.2



Re: [PATCH 3/7] NFC: nfcmrvl: do not use device-managed resources

2017-03-29 Thread Johan Hovold
On Wed, Mar 29, 2017 at 06:21:08PM +0200, Johan Hovold wrote:
> This specifically fixes a NULL-pointer dereference when using the n_nci
> line discipline on one end of a Unix98 pty as well as resource leaks in
> the registration error paths.

I noticed I forgot to actually fix up the error paths, so will send a v2
tomorrow.

Johan


[PATCH 3/7] NFC: nfcmrvl: do not use device-managed resources

2017-03-29 Thread Johan Hovold
This specifically fixes a NULL-pointer dereference when using the n_nci
line discipline on one end of a Unix98 pty as well as resource leaks in
the registration error paths.

Device-managed resources is a bad fit for this driver as devices can be
registered from the n_nci line discipline. Firstly, a tty may not even
have a corresponding device (should it be part of a Unix98 pty)
something which would lead to a NULL-pointer dereference when
registering resources.

Secondly, if the tty has a class device, its lifetime exceeds that of
the line discipline, which means that resources would leak every time
the line discipline is closed (or if registration fails).

Currently, the devres interface was only being used to request a reset
gpio despite the fact that it was already explicitly freed in
nfcmrvl_nci_unregister_dev() (along with the private data), something
which also prevented the resource leak at close.

Fixes: b2fe288eac72 ("NFC: nfcmrvl: free reset gpio")
Fixes: 4a2b947f56b3 ("NFC: nfcmrvl: add chip reset management")
Cc: stable  # 4.2: b2fe288eac72
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/main.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 51c8240a1672..8d4294fc3005 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -124,10 +124,9 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
memcpy(&priv->config, pdata, sizeof(*pdata));
 
if (priv->config.reset_n_io) {
-   rc = devm_gpio_request_one(dev,
-  priv->config.reset_n_io,
-  GPIOF_OUT_INIT_LOW,
-  "nfcmrvl_reset_n");
+   rc = gpio_request_one(priv->config.reset_n_io,
+ GPIOF_OUT_INIT_LOW,
+ "nfcmrvl_reset_n");
if (rc < 0)
nfc_err(dev, "failed to request reset_n io\n");
}
@@ -195,7 +194,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private 
*priv)
nfcmrvl_fw_dnld_deinit(priv);
 
if (priv->config.reset_n_io)
-   devm_gpio_free(priv->dev, priv->config.reset_n_io);
+   gpio_free(priv->config.reset_n_io);
 
nci_unregister_device(ndev);
nci_free_device(ndev);
-- 
2.12.2



[PATCH 2/7] NFC: nfcmrvl_uart: add missing tty-device sanity check

2017-03-29 Thread Johan Hovold
Make sure to check the tty-device pointer before trying to access the
parent device to avoid dereferencing a NULL-pointer when the tty is one
end of a Unix98 pty.

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Cc: stable  # 4.2
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/uart.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 83a99e38e7bd..6c0c301611c4 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -109,6 +109,7 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
struct nfcmrvl_private *priv;
struct nfcmrvl_platform_data *pdata = NULL;
struct nfcmrvl_platform_data config;
+   struct device *dev = nu->tty->dev;
 
/*
 * Platform data cannot be used here since usually it is already used
@@ -116,9 +117,8 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
 * and check if DT entries were added.
 */
 
-   if (nu->tty->dev->parent && nu->tty->dev->parent->of_node)
-   if (nfcmrvl_uart_parse_dt(nu->tty->dev->parent->of_node,
- &config) == 0)
+   if (dev && dev->parent && dev->parent->of_node)
+   if (nfcmrvl_uart_parse_dt(dev->parent->of_node, &config) == 0)
pdata = &config;
 
if (!pdata) {
@@ -131,7 +131,7 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
}
 
priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_UART, nu, &uart_ops,
-   nu->tty->dev, pdata);
+   dev, pdata);
if (IS_ERR(priv))
return PTR_ERR(priv);
 
-- 
2.12.2



[PATCH 1/7] NFC: fix broken device allocation

2017-03-29 Thread Johan Hovold
Commit 7eda8b8e9677 ("NFC: Use IDR library to assing NFC devices IDs")
moved device-id allocation and struct-device initialisation from
nfc_allocate_device() to nfc_register_device().

This broke just about every nfc-device-registration error path, which
continue to call nfc_free_device() that tries to put the device
reference of the now uninitialised (but zeroed) struct device:

kobject: '(null)' (ce316420): is not initialized, yet kobject_put() is being 
called.

The late struct-device initialisation also meant that various work
queues whose names are derived from the nfc device name were also
misnamed:

  421 root 0 SW<  [(null)_nci_cmd_]
  422 root 0 SW<  [(null)_nci_rx_w]
  423 root 0 SW<  [(null)_nci_tx_w]

Move the id-allocation and struct-device initialisation back to
nfc_allocate_device() and fix up the single call site which did not use
nfc_free_device() in its registration error path.

Fixes: 7eda8b8e9677 ("NFC: Use IDR library to assing NFC devices IDs")
Cc: stable  # 3.8
Cc: Samuel Ortiz 
Signed-off-by: Johan Hovold 
---
 net/nfc/core.c | 31 ++-
 net/nfc/nci/core.c |  3 +--
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/nfc/core.c b/net/nfc/core.c
index 122bb81da918..5cf33df888c3 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -982,6 +982,8 @@ static void nfc_release(struct device *d)
kfree(se);
}
 
+   ida_simple_remove(&nfc_index_ida, dev->idx);
+
kfree(dev);
 }
 
@@ -1056,6 +1058,7 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
int tx_headroom, int tx_tailroom)
 {
struct nfc_dev *dev;
+   int rc;
 
if (!ops->start_poll || !ops->stop_poll || !ops->activate_target ||
!ops->deactivate_target || !ops->im_transceive)
@@ -1068,6 +1071,15 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
if (!dev)
return NULL;
 
+   rc = ida_simple_get(&nfc_index_ida, 0, 0, GFP_KERNEL);
+   if (rc < 0)
+   goto err_free_dev;
+   dev->idx = rc;
+
+   dev->dev.class = &nfc_class;
+   dev_set_name(&dev->dev, "nfc%d", dev->idx);
+   device_initialize(&dev->dev);
+
dev->ops = ops;
dev->supported_protocols = supported_protocols;
dev->tx_headroom = tx_headroom;
@@ -1090,6 +1102,11 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
}
 
return dev;
+
+err_free_dev:
+   kfree(dev);
+
+   return ERR_PTR(rc);
 }
 EXPORT_SYMBOL(nfc_allocate_device);
 
@@ -1104,14 +1121,6 @@ int nfc_register_device(struct nfc_dev *dev)
 
pr_debug("dev_name=%s\n", dev_name(&dev->dev));
 
-   dev->idx = ida_simple_get(&nfc_index_ida, 0, 0, GFP_KERNEL);
-   if (dev->idx < 0)
-   return dev->idx;
-
-   dev->dev.class = &nfc_class;
-   dev_set_name(&dev->dev, "nfc%d", dev->idx);
-   device_initialize(&dev->dev);
-
mutex_lock(&nfc_devlist_mutex);
nfc_devlist_generation++;
rc = device_add(&dev->dev);
@@ -1149,12 +1158,10 @@ EXPORT_SYMBOL(nfc_register_device);
  */
 void nfc_unregister_device(struct nfc_dev *dev)
 {
-   int rc, id;
+   int rc;
 
pr_debug("dev_name=%s\n", dev_name(&dev->dev));
 
-   id = dev->idx;
-
if (dev->rfkill) {
rfkill_unregister(dev->rfkill);
rfkill_destroy(dev->rfkill);
@@ -1179,8 +1186,6 @@ void nfc_unregister_device(struct nfc_dev *dev)
nfc_devlist_generation++;
device_del(&dev->dev);
mutex_unlock(&nfc_devlist_mutex);
-
-   ida_simple_remove(&nfc_index_ida, id);
 }
 EXPORT_SYMBOL(nfc_unregister_device);
 
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 61fff422424f..85a3d9ed4c29 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1173,8 +1173,7 @@ struct nci_dev *nci_allocate_device(struct nci_ops *ops,
return ndev;
 
 free_nfc:
-   kfree(ndev->nfc_dev);
-
+   nfc_free_device(ndev->nfc_dev);
 free_nci:
kfree(ndev);
return NULL;
-- 
2.12.2



[PATCH 0/7] NFC: fix device allocation and nfcmrvl crashes

2017-03-29 Thread Johan Hovold
This started out with the observation that the nfcmrvl_uart driver
unconditionally dereferenced the tty class device despite the fact that
not every tty has an associated struct device (Unix98 ptys). Some
further changes were needed in the common nfcmrvl code to fully address
this, some of which also incidentally fixed a few related bugs (e.g.
resource leaks in error paths).

While fixing this I stumbled over a regression in NFC core that lead to
broken registration error paths and misnamed workqueues.

Note that this has only been tested by configuring the n_hci line
discipline for different ttys without any actual NFC hardware connected.

Johan


Johan Hovold (7):
  NFC: fix broken device allocation
  NFC: nfcmrvl_uart: add missing tty-device sanity check
  NFC: nfcmrvl: do not use device-managed resources
  NFC: nfcmrvl: use nfc-device for firmware download
  NFC: nfcmrvl: fix firmware-management initialisation
  NFC: nfcmrvl_uart: fix device-node leak during probe
  NFC: nfcmrvl_usb: use interface as phy device

 drivers/nfc/nfcmrvl/fw_dnld.c |  7 +--
 drivers/nfc/nfcmrvl/main.c| 25 +
 drivers/nfc/nfcmrvl/uart.c| 11 +++
 drivers/nfc/nfcmrvl/usb.c |  4 +---
 net/nfc/core.c| 31 ++-
 net/nfc/nci/core.c|  3 +--
 6 files changed, 45 insertions(+), 36 deletions(-)

-- 
2.12.2



[PATCH 7/7] NFC: nfcmrvl_usb: use interface as phy device

2017-03-29 Thread Johan Hovold
Use the USB-interface rather than parent USB-device device, which is
what this driver binds to, when registering the nci device.

Note that using the right device is important when dealing with device-
managed resources as the interface can be unbound independently of the
parent device.

Also note that private device pointer had already been set by
nfcmrvl_nci_register_dev() so the redundant assignment can therefore be
removed.

Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/usb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index 585a0f20835b..728b3321842d 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -341,15 +341,13 @@ static int nfcmrvl_probe(struct usb_interface *intf,
init_usb_anchor(&drv_data->deferred);
 
priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_USB, drv_data, &usb_ops,
-   &drv_data->udev->dev, &config);
+   &intf->dev, &config);
if (IS_ERR(priv))
return PTR_ERR(priv);
 
drv_data->priv = priv;
drv_data->priv->support_fw_dnld = false;
 
-   priv->dev = &drv_data->udev->dev;
-
usb_set_intfdata(intf, drv_data);
 
return 0;
-- 
2.12.2



[PATCH 4/7] NFC: nfcmrvl: use nfc-device for firmware download

2017-03-29 Thread Johan Hovold
Use the nfc- rather than phy-device in firmware-management code that
needs a valid struct device.

This specifically fixes a NULL-pointer dereference in
nfcmrvl_fw_dnld_init() during registration when the underlying tty is
one end of a Unix98 pty.

Note that the driver still uses the phy device for any debugging, which
is fine for now.

Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Cc: stable  # 4.4
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/fw_dnld.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
index f8dcdf4b24f6..af62c4c854f3 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -459,7 +459,7 @@ int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv)
 
INIT_WORK(&priv->fw_dnld.rx_work, fw_dnld_rx_work);
snprintf(name, sizeof(name), "%s_nfcmrvl_fw_dnld_rx_wq",
-dev_name(priv->dev));
+dev_name(&priv->ndev->nfc_dev->dev));
priv->fw_dnld.rx_wq = create_singlethread_workqueue(name);
if (!priv->fw_dnld.rx_wq)
return -ENOMEM;
@@ -496,6 +496,7 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char 
*firmware_name)
 {
struct nfcmrvl_private *priv = nci_get_drvdata(ndev);
struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+   int res;
 
if (!priv->support_fw_dnld)
return -ENOTSUPP;
@@ -511,7 +512,9 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char 
*firmware_name)
 */
 
/* Retrieve FW binary */
-   if (request_firmware(&fw_dnld->fw, firmware_name, priv->dev) < 0) {
+   res = request_firmware(&fw_dnld->fw, firmware_name,
+  &ndev->nfc_dev->dev);
+   if (res < 0) {
nfc_err(priv->dev, "failed to retrieve FW %s", firmware_name);
return -ENOENT;
}
-- 
2.12.2



[PATCH 6/7] NFC: nfcmrvl_uart: fix device-node leak during probe

2017-03-29 Thread Johan Hovold
Make sure to release the device-node reference when done parsing the
node.

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/uart.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 6c0c301611c4..91162f8e0366 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -84,6 +84,7 @@ static int nfcmrvl_uart_parse_dt(struct device_node *node,
ret = nfcmrvl_parse_dt(matched_node, pdata);
if (ret < 0) {
pr_err("Failed to get generic entries\n");
+   of_node_put(matched_node);
return ret;
}
 
@@ -97,6 +98,8 @@ static int nfcmrvl_uart_parse_dt(struct device_node *node,
else
pdata->break_control = 0;
 
+   of_node_put(matched_node);
+
return 0;
 }
 
-- 
2.12.2



[PATCH 5/7] NFC: nfcmrvl: fix firmware-management initialisation

2017-03-29 Thread Johan Hovold
The nci-device was never deregistered in the event that
fw-initialisation failed.

Fix this by moving the firmware initialisation before device
registration since the firmware work queue should be available before
registering.

Note that this depends on a recent fix that moved device-name
initialisation back to to nci_allocate_device() as the
firmware-workqueue name is now derived from the nfc-device name.

Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Cc: stable  # 4.4
Cc: Vincent Cuissard 
Signed-off-by: Johan Hovold 
---
 drivers/nfc/nfcmrvl/main.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 8d4294fc3005..f20220bab6b6 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -156,26 +156,28 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum 
nfcmrvl_phy phy,
goto error;
}
 
+   rc = nfcmrvl_fw_dnld_init(priv);
+   if (rc) {
+   nfc_err(dev, "failed to initialize FW download %d\n", rc);
+   goto error_free_dev;
+   }
+
nci_set_drvdata(priv->ndev, priv);
 
rc = nci_register_device(priv->ndev);
if (rc) {
nfc_err(dev, "nci_register_device failed %d\n", rc);
-   goto error_free_dev;
+   goto error_fw_dnld_deinit;
}
 
/* Ensure that controller is powered off */
nfcmrvl_chip_halt(priv);
 
-   rc = nfcmrvl_fw_dnld_init(priv);
-   if (rc) {
-   nfc_err(dev, "failed to initialize FW download %d\n", rc);
-   goto error_free_dev;
-   }
-
nfc_info(dev, "registered with nci successfully\n");
return priv;
 
+error_fw_dnld_deinit:
+   nfcmrvl_fw_dnld_deinit(priv);
 error_free_dev:
nci_free_device(priv->ndev);
 error:
-- 
2.12.2



Re: [2/2] zd1211rw: fix NULL-deref at probe

2017-03-22 Thread Johan Hovold
On Wed, Mar 22, 2017 at 03:02:12PM +0200, Kalle Valo wrote:
> Johan Hovold  writes:
> 
> > On Wed, Mar 22, 2017 at 09:04:15AM +, Kalle Valo wrote:
> >> Johan Hovold  wrote:
> >> > Make sure to check the number of endpoints to avoid dereferencing a
> >> > NULL-pointer or accessing memory beyond the endpoint array should a
> >> > malicious device lack the expected endpoints.
> >> > 
> >> > Fixes: a1030e92c150 ("[PATCH] zd1211rw: Convert installer CDROM
> >> > device into WLAN device")
> >> > Cc: Daniel Drake 
> >> > Signed-off-by: Johan Hovold 
> >> 
> >> Patch applied to wireless-drivers-next.git, thanks.
> >> 
> >> ca260ece6a57 zd1211rw: fix NULL-deref at probe
> >
> > What about patch 1/2 which fixes the same bug (literally copied from the
> > zd1211rw driver)?
> 
> I will apply that to my separate ath.git tree, just didn't get to your
> patch yet.

Ah, ok. 

> > And as these fixes should be backported to stable (I left out the tag
> > for networking drivers)
> 
> Actually for wireless drivers you should add the stable tag.

Alright, will do in the future.

> > why only apply to -next?
> 
> I didn't see that the fix was important enough for 4.11.

Ok, but fixes for these types of crashes that can be triggered by a
malicious device have typically gone into the current -rc (a couple just
went in through the net tree for example).

Thanks,
Johan


Re: [2/2] zd1211rw: fix NULL-deref at probe

2017-03-22 Thread Johan Hovold
On Wed, Mar 22, 2017 at 09:04:15AM +, Kalle Valo wrote:
> Johan Hovold  wrote:
> > Make sure to check the number of endpoints to avoid dereferencing a
> > NULL-pointer or accessing memory beyond the endpoint array should a
> > malicious device lack the expected endpoints.
> > 
> > Fixes: a1030e92c150 ("[PATCH] zd1211rw: Convert installer CDROM device into 
> > WLAN device")
> > Cc: Daniel Drake 
> > Signed-off-by: Johan Hovold 
> 
> Patch applied to wireless-drivers-next.git, thanks.
> 
> ca260ece6a57 zd1211rw: fix NULL-deref at probe

What about patch 1/2 which fixes the same bug (literally copied from the
zd1211rw driver)?

And as these fixes should be backported to stable (I left out the tag
for networking drivers), why only apply to -next?

Thanks,
Johan


[PATCH 1/2] wireless: ath9k_htc: fix NULL-deref at probe

2017-03-13 Thread Johan Hovold
Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer or accessing memory beyond the endpoint array should a
malicious device lack the expected endpoints.

Fixes: 36bcce430657 ("ath9k_htc: Handle storage devices")
Cc: Sujith Manoharan 
Signed-off-by: Johan Hovold 
---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c 
b/drivers/net/wireless/ath/ath9k/hif_usb.c
index de2d212f39ec..9206955e865a 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -1219,6 +1219,9 @@ static int send_eject_command(struct usb_interface 
*interface)
u8 bulk_out_ep;
int r;
 
+   if (iface_desc->desc.bNumEndpoints < 2)
+   return -ENODEV;
+
/* Find bulk out endpoint */
for (r = 1; r >= 0; r--) {
endpoint = &iface_desc->endpoint[r].desc;
-- 
2.12.0



[PATCH 2/2] wireless: zd1211rw: fix NULL-deref at probe

2017-03-13 Thread Johan Hovold
Make sure to check the number of endpoints to avoid dereferencing a
NULL-pointer or accessing memory beyond the endpoint array should a
malicious device lack the expected endpoints.

Fixes: a1030e92c150 ("[PATCH] zd1211rw: Convert installer CDROM device
into WLAN device")
Cc: Daniel Drake 

Signed-off-by: Johan Hovold 
---
 drivers/net/wireless/zydas/zd1211rw/zd_usb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c 
b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
index c5effd6c6be9..01ca1d57b3d9 100644
--- a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
@@ -1278,6 +1278,9 @@ static int eject_installer(struct usb_interface *intf)
u8 bulk_out_ep;
int r;
 
+   if (iface_desc->desc.bNumEndpoints < 2)
+   return -ENODEV;
+
/* Find bulk out endpoint */
for (r = 1; r >= 0; r--) {
endpoint = &iface_desc->endpoint[r].desc;
-- 
2.12.0



Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage

2015-12-14 Thread Johan Hovold
On Wed, Dec 09, 2015 at 03:46:00PM +0100, Linus Walleij wrote:
> On Wed, Dec 9, 2015 at 2:44 PM, Russell King - ARM Linux
>  wrote:

> > On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote:
> >> Because we want to have a proper userspace ABI for GPIO chips,
> >> which involves using a character device that the user opens
> >> and closes. While the character device is open, the underlying
> >> kernel objects must not go away.
> >
> > Okay, so you stop the gpio_chip struct from going away.
> 
> Actually I was going to create a new struct gpio_device, but yes.
> 
> > What
> > about the code which is called via gpio_chip - say, if userspace
> > keeps the chardev open, and someone rmmod's the driver providing
> > the GPIO driver.
> 
> The idea was that when what is today gpiochip_remove() is called by the
> gpiochip driver, the static data pointer to the vtable with all
> callbacks is set to NULL (in some safe way), and the code avoids
> calling these, so the device goes numb.
> 
> I think you give me the right solution to this "safe way" below,
> thanks!
> 
> > I'm not sure that splitting up objects in this way really solves
> > anything at all.  Yes, it divorses the driver's private data from
> > the subsystem data, but is that really an advantage?
> 
> I like the design pattern you describe below, and
> I have no strong opinion on it. If there is a precedent in the kernel
> to do it with a separate alloc_foo() function I can do that.
> 
> (Would like Greg's and/or Johan's opinion on this though.)

I'd prefer separating allocation and registration rather than using a
setup callback.

> > Things get a little more complex with gpio, because there's the
> > issue that some methods are spinlocked while others can take
> > semaphores, but it should be possible to come up with a solution
> > to that - maybe an atomic_t which is incremented whenever we're
> > in some operation provided it's >= 0 (otherwise it fails), and
> > decremented when the operation completes.  We can then control
> > in the unregistration path further GPIO accesses, and also
> > prevent new accesses occuring by setting the atomic_t to -1.
> > This shouldn't require any additional locking in any path.  It
> > does mean that the unregistration path needs careful thought to
> > ensure that when we set it to -1, we wait for it to be dropped
> > by the appropriate amount.
> 
> Yeah we will both in spinlocks/hotpath and
> semaphores/mutexes/slowpath in these ops for sure :/
> 
> The atomic hack should work: I understand that you mean
> we read it and set it to -1 and if it was 2 wait for it to
> become -3, then conclude unregistration with callbacks
> numbed.

Ok, but let's take a step back. So you have all this in place and a
consumer calls gpiod_get_value() that returns an errno because the device
is gone. Note that this wasn't even possible before e20538b82f1f ("gpio:
Propagate errors from chip->get()") that went into *v4.3*, and I assume
most drivers would need to be updated to even handle that that gpio
call, and all future calls, are now suddenly failing.

So this ties into the generic problem of inter-device dependencies. Does
it even make sense to keep the consumer around, now that its gpio
resources have gone away?  

If your current concern is a new gpio chardev interface, perhaps solving
only that use case in the way that Dimitry suggested elsewhere in this
thread is what you should be doing.

> Then there is a particular case that occurs with USB or similar
> pluggable buses, where there is a glitch or powercycle on the
> bus, and the same device goes away and comes back in
> a few milliseconds, and that means it should reattach to the
> character device that is already open.

No, that does not follow. A USB device being disconnected and
reconnected is considered to be a new device. All state is gone, and the
dangling character device will be unusable.

> Making that work is however non-trivial :(

Fortunately, you don't need to worry about that.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html