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

[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 <cmmpsan...@gmail.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> Cc: Stephen Hemminger <step...@networkplumber.org>
> Cc: stable <sta...@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> ---
> 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 <jo...@kernel.org>

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

[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

[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 <sta...@vger.kernel.org> # 3.13
Cc: Heikki Krogerus <heikki.kroge...@linux.intel.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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(>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

[Johan Hovold]

On Mon, Nov 13, 2017 at 08:51:49AM +, Kalle Valo wrote:
> Johan Hovold <jo...@kernel.org> 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 <sta...@vger.kernel.org> # 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 <loic.poul...@linaro.org>
> > Signed-off-by: Johan Hovold <jo...@kernel.org>
> 
> Thanks, I'll queue this to v4.15.

Johan

[PATCH] wcn36xx: fix iris child-node lookup

[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 <sta...@vger.kernel.org> # 4.14
Cc: Loic Poulain <loic.poul...@linaro.org>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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

[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 <sta...@vger.kernel.org> # 3.8
> > Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> > Signed-off-by: Johan Hovold <jo...@kernel.org>

> 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

[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 <sta...@vger.kernel.org>   # 3.8
> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> Signed-off-by: Johan Hovold <jo...@kernel.org>

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

[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 <sta...@vger.kernel.org> # 3.8
Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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

[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(>resp_q);
>   2640  skb_queue_head_init(>fragment_skb);
>   2641  
>   2642  INIT_LIST_HEAD(>cmd_queue);
>   2643  
>   2644  priv->nfc_dev = nfc_allocate_device(_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

[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

[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

[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 <jo...@kernel.org>
---
 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

[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

[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

[Johan Hovold]

On Mon, Apr 03, 2017 at 01:21:08PM +, Kalle Valo wrote:
> Johan Hovold <jo...@kernel.org> writes:
> 
> > On Mon, Apr 03, 2017 at 01:02:28PM +, Kalle Valo wrote:
> >> Kalle Valo <kv...@codeaurora.org> writes:
> >> 
> >> > Johan Hovold <jo...@kernel.org> 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 <sujith.manoha...@atheros.com>
> >> >>> Signed-off-by: Johan Hovold <jo...@kernel.org>
> >> >>
> >> >> 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 <sta...@vger.kernel.org> # 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

[Johan Hovold]

On Mon, Apr 03, 2017 at 01:02:28PM +, Kalle Valo wrote:
> Kalle Valo <kv...@codeaurora.org> writes:
> 
> > Johan Hovold <jo...@kernel.org> 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 <sujith.manoha...@atheros.com>
> >>> Signed-off-by: Johan Hovold <jo...@kernel.org>
> >>
> >> 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 <sta...@vger.kernel.org> # 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

[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 <sujith.manoha...@atheros.com>
> Signed-off-by: Johan Hovold <jo...@kernel.org>

Is this one still in your queue, Kalle?

As I mentioned earlier, I should have added a

Cc: stable <sta...@vger.kernel.org> # 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

[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 <sta...@vger.kernel.org> # 3.8
Cc: Samuel Ortiz <sa...@linux.intel.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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(_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(_index_ida, 0, 0, GFP_KERNEL);
+   if (rc < 0)
+   goto err_free_dev;
+   dev->idx = rc;
+
+   dev->dev.class = _class;
+   dev_set_name(>dev, "nfc%d", dev->idx);
+   device_initialize(>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->idx = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
-   if (dev->idx < 0)
-   return dev->idx;
-
-   dev->dev.class = _class;
-   dev_set_name(>dev, "nfc%d", dev->idx);
-   device_initialize(>dev);
-
mutex_lock(_devlist_mutex);
nfc_devlist_generation++;
rc = device_add(>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));
 
-   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);
mutex_unlock(_devlist_mutex);
-
-   ida_simple_remove(_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

[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

[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 <sta...@vger.kernel.org> # 4.2: b2fe288eac72
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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(>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

[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 <sta...@vger.kernel.org> # 4.2
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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,
- ) == 0)
+   if (dev && dev->parent && dev->parent->of_node)
+   if (nfcmrvl_uart_parse_dt(dev->parent->of_node, ) == 0)
pdata = 
 
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, _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

[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 <jo...@kernel.org>
---
 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(>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

[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 <jo...@kernel.org>
---
 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(_data->deferred);
 
priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_USB, drv_data, _ops,
-   _data->udev->dev, );
+   >dev, );
if (IS_ERR(priv))
return PTR_ERR(priv);
 
drv_data->priv = priv;
drv_data->priv->support_fw_dnld = false;
 
-   priv->dev = _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

[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 <sta...@vger.kernel.org> # 4.4
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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(>fw_dnld.rx_work, fw_dnld_rx_work);
snprintf(name, sizeof(name), "%s_nfcmrvl_fw_dnld_rx_wq",
-dev_name(priv->dev));
+dev_name(>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 = >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(_dnld->fw, firmware_name, priv->dev) < 0) {
+   res = request_firmware(_dnld->fw, firmware_name,
+  >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

[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 <sta...@vger.kernel.org> # 4.4
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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

[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 <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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

[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

[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 <sta...@vger.kernel.org> # 4.2: b2fe288eac72
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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(>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

[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 <sta...@vger.kernel.org> # 4.2
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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,
- ) == 0)
+   if (dev && dev->parent && dev->parent->of_node)
+   if (nfcmrvl_uart_parse_dt(dev->parent->of_node, ) == 0)
pdata = 
 
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, _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

[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 <sta...@vger.kernel.org> # 3.8
Cc: Samuel Ortiz <sa...@linux.intel.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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(_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(_index_ida, 0, 0, GFP_KERNEL);
+   if (rc < 0)
+   goto err_free_dev;
+   dev->idx = rc;
+
+   dev->dev.class = _class;
+   dev_set_name(>dev, "nfc%d", dev->idx);
+   device_initialize(>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->idx = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
-   if (dev->idx < 0)
-   return dev->idx;
-
-   dev->dev.class = _class;
-   dev_set_name(>dev, "nfc%d", dev->idx);
-   device_initialize(>dev);
-
mutex_lock(_devlist_mutex);
nfc_devlist_generation++;
rc = device_add(>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));
 
-   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);
mutex_unlock(_devlist_mutex);
-
-   ida_simple_remove(_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

[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

[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 <jo...@kernel.org>
---
 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(_data->deferred);
 
priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_USB, drv_data, _ops,
-   _data->udev->dev, );
+   >dev, );
if (IS_ERR(priv))
return PTR_ERR(priv);
 
drv_data->priv = priv;
drv_data->priv->support_fw_dnld = false;
 
-   priv->dev = _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

[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 <sta...@vger.kernel.org> # 4.4
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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(>fw_dnld.rx_work, fw_dnld_rx_work);
snprintf(name, sizeof(name), "%s_nfcmrvl_fw_dnld_rx_wq",
-dev_name(priv->dev));
+dev_name(>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 = >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(_dnld->fw, firmware_name, priv->dev) < 0) {
+   res = request_firmware(_dnld->fw, firmware_name,
+  >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

[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 <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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

[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 <sta...@vger.kernel.org> # 4.4
Cc: Vincent Cuissard <cuiss...@marvell.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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

[Johan Hovold]

On Wed, Mar 22, 2017 at 03:02:12PM +0200, Kalle Valo wrote:
> Johan Hovold <jo...@kernel.org> writes:
> 
> > On Wed, Mar 22, 2017 at 09:04:15AM +, Kalle Valo wrote:
> >> Johan Hovold <jo...@kernel.org> 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 <d...@gentoo.org>
> >> > Signed-off-by: Johan Hovold <jo...@kernel.org>
> >> 
> >> 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

[Johan Hovold]

On Wed, Mar 22, 2017 at 09:04:15AM +, Kalle Valo wrote:
> Johan Hovold <jo...@kernel.org> 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 <d...@gentoo.org>
> > Signed-off-by: Johan Hovold <jo...@kernel.org>
> 
> 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

[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 <sujith.manoha...@atheros.com>
Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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 = _desc->endpoint[r].desc;
-- 
2.12.0

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

[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 <d...@gentoo.org>

Signed-off-by: Johan Hovold <jo...@kernel.org>
---
 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 = _desc->endpoint[r].desc;
-- 
2.12.0

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

[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