diff mbox

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

Message ID 20180520131946.GA7325@kroah.com (mailing list archive)
State Deferred
Delegated to: Samuel Ortiz
Headers show

Commit Message

Greg Kroah-Hartman May 20, 2018, 1:19 p.m. UTC
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 <cmmpsantos@gmail.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@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

 drivers/nfc/pn533/usb.c |   42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

Comments

Johan Hovold May 21, 2018, 8:38 a.m. UTC | #1
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 <cmmpsantos@gmail.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@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 <johan@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
Greg Kroah-Hartman May 31, 2018, 10:38 a.m. UTC | #2
On Mon, May 21, 2018 at 10:38:25AM +0200, Johan Hovold wrote:
> 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 <cmmpsantos@gmail.com>
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: stable <stable@vger.kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@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 <johan@kernel.org>

Thanks for the review.

> 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.

Yeah, this whole thing is a mess and I really don't want to do much more
to the driver without having the hardware to test with it.  So I'll just
leave it as-is for now :)

greg k-h
Mark Greer June 1, 2018, 4:06 a.m. UTC | #3
On Thu, May 31, 2018 at 12:38:48PM +0200, Greg Kroah-Hartman wrote:
> On Mon, May 21, 2018 at 10:38:25AM +0200, Johan Hovold wrote:
> > 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 <cmmpsantos@gmail.com>
> > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > Cc: stable <stable@vger.kernel.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@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 <johan@kernel.org>
> 
> Thanks for the review.
> 
> > 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.
> 
> Yeah, this whole thing is a mess and I really don't want to do much more
> to the driver without having the hardware to test with it.  So I'll just
> leave it as-is for now :)

Hi Greg.

I have a pn533 USB stick, a trf7970-based board, and some nfc tags so I
can help you test as long as you don't mind multi-day turn-around times.
Alternatively, I can set something up and give you remote access, if you
prefer that.

Let me know.

Mark
--
Greg Kroah-Hartman June 1, 2018, 8:03 a.m. UTC | #4
On Thu, May 31, 2018 at 09:06:32PM -0700, Mark Greer wrote:
> On Thu, May 31, 2018 at 12:38:48PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, May 21, 2018 at 10:38:25AM +0200, Johan Hovold wrote:
> > > 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 <cmmpsantos@gmail.com>
> > > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > > Cc: stable <stable@vger.kernel.org>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@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 <johan@kernel.org>
> > 
> > Thanks for the review.
> > 
> > > 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.
> > 
> > Yeah, this whole thing is a mess and I really don't want to do much more
> > to the driver without having the hardware to test with it.  So I'll just
> > leave it as-is for now :)
> 
> Hi Greg.
> 
> I have a pn533 USB stick, a trf7970-based board, and some nfc tags so I
> can help you test as long as you don't mind multi-day turn-around times.
> Alternatively, I can set something up and give you remote access, if you
> prefer that.
> 
> Let me know.

multi-day turn arounds are fine.  But given that this driver has been
broken for a very long time with no one noticing, I doubt that there are
many, if any, real users out there to make it worth working on at the
moment to fix up minor code quality issues :(

thanks,

greg k-h
Samuel Ortiz June 9, 2018, 9:54 a.m. UTC | #5
Greg,

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 <cmmpsantos@gmail.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@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
> 
>  drivers/nfc/pn533/usb.c |   42 ++++++++++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 12 deletions(-)
I can take that one through nfc-next, and the few subsequent patches
from Hans and Julia. Please let me know what's more convenient for you.

Cheers,
Samuel.
Greg Kroah-Hartman June 9, 2018, 2:01 p.m. UTC | #6
On Sat, Jun 09, 2018 at 11:54:01AM +0200, Samuel Ortiz wrote:
> Greg,
> 
> 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 <cmmpsantos@gmail.com>
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: stable <stable@vger.kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@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
> > 
> >  drivers/nfc/pn533/usb.c |   42 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 30 insertions(+), 12 deletions(-)
> I can take that one through nfc-next, and the few subsequent patches
> from Hans and Julia. Please let me know what's more convenient for you.

This patch is already in Linus's tree, along with whatever applied from
Julia's patch series.  Hans needs to rebase and resend anyway, so either
tree is fine once 4.18-rc1 is out.

thanks,

greg k-h
diff mbox

Patch

--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -62,6 +62,9 @@  struct pn533_usb_phy {
 	struct urb *out_urb;
 	struct urb *in_urb;
 
+	struct urb *ack_urb;
+	u8 *ack_buffer;
+
 	struct pn533 *priv;
 };
 
@@ -150,13 +153,16 @@  static int pn533_usb_send_ack(struct pn5
 	struct pn533_usb_phy *phy = dev->phy;
 	static const u8 ack[6] = {0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
 	/* spec 7.1.1.3:  Preamble, SoPC (2), ACK Code (2), Postamble */
-	int rc;
 
-	phy->out_urb->transfer_buffer = (u8 *)ack;
-	phy->out_urb->transfer_buffer_length = sizeof(ack);
-	rc = usb_submit_urb(phy->out_urb, flags);
+	if (!phy->ack_buffer) {
+		phy->ack_buffer = kmemdup(ack, sizeof(ack), flags);
+		if (!phy->ack_buffer)
+			return -ENOMEM;
+	}
 
-	return rc;
+	phy->ack_urb->transfer_buffer = phy->ack_buffer;
+	phy->ack_urb->transfer_buffer_length = sizeof(ack);
+	return usb_submit_urb(phy->ack_urb, flags);
 }
 
 static int pn533_usb_send_frame(struct pn533 *dev,
@@ -375,26 +381,31 @@  static int pn533_acr122_poweron_rdr(stru
 	/* Power on th reader (CCID cmd) */
 	u8 cmd[10] = {PN533_ACR122_PC_TO_RDR_ICCPOWERON,
 		      0, 0, 0, 0, 0, 0, 3, 0, 0};
+	char *buffer;
+	int transferred;
 	int rc;
 	void *cntx;
 	struct pn533_acr122_poweron_rdr_arg arg;
 
 	dev_dbg(&phy->udev->dev, "%s\n", __func__);
 
+	buffer = kmemdup(cmd, sizeof(cmd), GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
 	init_completion(&arg.done);
 	cntx = phy->in_urb->context;  /* backup context */
 
 	phy->in_urb->complete = pn533_acr122_poweron_rdr_resp;
 	phy->in_urb->context = &arg;
 
-	phy->out_urb->transfer_buffer = cmd;
-	phy->out_urb->transfer_buffer_length = sizeof(cmd);
-
 	print_hex_dump_debug("ACR122 TX: ", DUMP_PREFIX_NONE, 16, 1,
 		       cmd, sizeof(cmd), false);
 
-	rc = usb_submit_urb(phy->out_urb, GFP_KERNEL);
-	if (rc) {
+	rc = usb_bulk_msg(phy->udev, phy->out_urb->pipe, buffer, sizeof(cmd),
+			  &transferred, 0);
+	kfree(buffer);
+	if (rc || (transferred != sizeof(cmd))) {
 		nfc_err(&phy->udev->dev,
 			"Reader power on cmd error %d\n", rc);
 		return rc;
@@ -490,8 +501,9 @@  static int pn533_usb_probe(struct usb_in
 
 	phy->in_urb = usb_alloc_urb(0, GFP_KERNEL);
 	phy->out_urb = usb_alloc_urb(0, GFP_KERNEL);
+	phy->ack_urb = usb_alloc_urb(0, GFP_KERNEL);
 
-	if (!phy->in_urb || !phy->out_urb)
+	if (!phy->in_urb || !phy->out_urb || !phy->ack_urb)
 		goto error;
 
 	usb_fill_bulk_urb(phy->in_urb, phy->udev,
@@ -501,7 +513,9 @@  static int pn533_usb_probe(struct usb_in
 	usb_fill_bulk_urb(phy->out_urb, phy->udev,
 			  usb_sndbulkpipe(phy->udev, out_endpoint),
 			  NULL, 0, pn533_send_complete, phy);
-
+	usb_fill_bulk_urb(phy->ack_urb, phy->udev,
+			  usb_sndbulkpipe(phy->udev, out_endpoint),
+			  NULL, 0, pn533_send_complete, phy);
 
 	switch (id->driver_info) {
 	case PN533_DEVICE_STD:
@@ -554,6 +568,7 @@  static int pn533_usb_probe(struct usb_in
 error:
 	usb_free_urb(phy->in_urb);
 	usb_free_urb(phy->out_urb);
+	usb_free_urb(phy->ack_urb);
 	usb_put_dev(phy->udev);
 	kfree(in_buf);
 
@@ -573,10 +588,13 @@  static void pn533_usb_disconnect(struct
 
 	usb_kill_urb(phy->in_urb);
 	usb_kill_urb(phy->out_urb);
+	usb_kill_urb(phy->ack_urb);
 
 	kfree(phy->in_urb->transfer_buffer);
 	usb_free_urb(phy->in_urb);
 	usb_free_urb(phy->out_urb);
+	usb_free_urb(phy->ack_urb);
+	kfree(phy->ack_buffer);
 
 	nfc_info(&interface->dev, "NXP PN533 NFC device disconnected\n");
 }