Re: [PATCH] usb: gadget: fix NULL pointer dereference
On Fri, Jan 17, 2014 at 05:04:55PM +0100, Michal Nazarewicz wrote: On Thu, Jan 16 2014, Andrzej Pietrasiewicz wrote: Fix possible NULL pointer dereference introduced in 219580e64f035bb9018dbb08d340f90b0ac50f8c usb: f_fs: check quirk to pad epout buf size when not aligned to maxpacketsize after 3.13-rc1. In cases we do wait with: wait_event_interruptible(epfile-wait, (ep = epfile-ep)); for endpoint to be enabled, functionfs_bind() has not been called yet and epfile-ffs-gadget is still NULL and the automatic variable 'gadget' has been initialized with NULL at the point of its definition. Later on it is used as a parameter to: usb_ep_align_maybe(gadget, ep-ep, len) which in turn dereferences it. This patch fixes it by moving the actual assignment to the local 'gadget' variable after the potential waiting has completed. Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com Acked-by: Michal Nazarewicz min...@mina86.com But since gadget is only used in the “if (!halt)” part of the code, could you simply move definition of the variable inside the if? should I wait for another revision ? -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb: gadget: fix NULL pointer dereference
On Tue, Feb 18, 2014 at 10:40:12AM -0600, Felipe Balbi wrote: On Fri, Jan 17, 2014 at 05:04:55PM +0100, Michal Nazarewicz wrote: On Thu, Jan 16 2014, Andrzej Pietrasiewicz wrote: Fix possible NULL pointer dereference introduced in 219580e64f035bb9018dbb08d340f90b0ac50f8c usb: f_fs: check quirk to pad epout buf size when not aligned to maxpacketsize after 3.13-rc1. In cases we do wait with: wait_event_interruptible(epfile-wait, (ep = epfile-ep)); for endpoint to be enabled, functionfs_bind() has not been called yet and epfile-ffs-gadget is still NULL and the automatic variable 'gadget' has been initialized with NULL at the point of its definition. Later on it is used as a parameter to: usb_ep_align_maybe(gadget, ep-ep, len) which in turn dereferences it. This patch fixes it by moving the actual assignment to the local 'gadget' variable after the potential waiting has completed. Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com Acked-by: Michal Nazarewicz min...@mina86.com But since gadget is only used in the “if (!halt)” part of the code, could you simply move definition of the variable inside the if? should I wait for another revision ? nevermind, you already sent it ;-) -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb: gadget: fix NULL pointer dereference
W dniu 18.02.2014 17:40, Felipe Balbi pisze: On Fri, Jan 17, 2014 at 05:04:55PM +0100, Michal Nazarewicz wrote: On Thu, Jan 16 2014, Andrzej Pietrasiewicz wrote: Fix possible NULL pointer dereference introduced in 219580e64f035bb9018dbb08d340f90b0ac50f8c usb: f_fs: check quirk to pad epout buf size when not aligned to maxpacketsize after 3.13-rc1. In cases we do wait with: wait_event_interruptible(epfile-wait, (ep = epfile-ep)); for endpoint to be enabled, functionfs_bind() has not been called yet and epfile-ffs-gadget is still NULL and the automatic variable 'gadget' has been initialized with NULL at the point of its definition. Later on it is used as a parameter to: usb_ep_align_maybe(gadget, ep-ep, len) which in turn dereferences it. This patch fixes it by moving the actual assignment to the local 'gadget' variable after the potential waiting has completed. Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com Acked-by: Michal Nazarewicz min...@mina86.com But since gadget is only used in the “if (!halt)” part of the code, could you simply move definition of the variable inside the if? should I wait for another revision ? It has already been submitted: http://www.spinics.net/lists/linux-usb/msg101199.html AP -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: fix NULL pointer dereference
On Thu, Jan 16 2014, Andrzej Pietrasiewicz wrote:
Fix possible NULL pointer dereference introduced in
219580e64f035bb9018dbb08d340f90b0ac50f8c
usb: f_fs: check quirk to pad epout buf size when not aligned to
maxpacketsize
after 3.13-rc1.
In cases we do wait with:
wait_event_interruptible(epfile-wait, (ep = epfile-ep));
for endpoint to be enabled, functionfs_bind() has not been called yet
and epfile-ffs-gadget is still NULL and the automatic variable 'gadget'
has been initialized with NULL at the point of its definition.
Later on it is used as a parameter to:
usb_ep_align_maybe(gadget, ep-ep, len)
which in turn dereferences it.
This patch fixes it by moving the actual assignment to the local 'gadget'
variable after the potential waiting has completed.
Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com
Acked-by: Michal Nazarewicz min...@mina86.com
But since gadget is only used in the “if (!halt)” part of the code,
could you simply move definition of the variable inside the if?
---
drivers/usb/gadget/f_fs.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index fffda61..e84b73b 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -587,7 +587,7 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file-private_data;
- struct usb_gadget *gadget = epfile-ffs-gadget;
+ struct usb_gadget *gadget;
struct ffs_ep *ep;
char *data = NULL;
ssize_t ret, data_len;
@@ -613,6 +613,11 @@ static ssize_t ffs_epfile_io(struct file *file,
goto error;
}
}
+ /*
+ * if we _do_ wait above, the epfile-ffs-gadget might be NULL
+ * before the waiting completes, so do not assign to 'gadget' earlier
+ */
+ gadget = epfile-ffs-gadget;
/* Do we halt? */
halt = !read == !epfile-in;
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz(o o)
ooo +--m...@google.com--xmpp:min...@jabber.org--ooO--(_)--Ooo--
signature.asc
Description: PGP signature
[PATCH] usb: gadget: fix NULL pointer dereference
Fix possible NULL pointer dereference introduced in
219580e64f035bb9018dbb08d340f90b0ac50f8c
usb: f_fs: check quirk to pad epout buf size when not aligned to
maxpacketsize
after 3.13-rc1.
In cases we do wait with:
wait_event_interruptible(epfile-wait, (ep = epfile-ep));
for endpoint to be enabled, functionfs_bind() has not been called yet
and epfile-ffs-gadget is still NULL and the automatic variable 'gadget'
has been initialized with NULL at the point of its definition.
Later on it is used as a parameter to:
usb_ep_align_maybe(gadget, ep-ep, len)
which in turn dereferences it.
This patch fixes it by moving the actual assignment to the local 'gadget'
variable after the potential waiting has completed.
Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com
---
@Michal: This has been detected with adb function implemented on top of
FunctionFS and the gadget itself has been set up with configfs.
adb's transport calls read (and usually blocks on it) very early and then
the problem manifests itself.
drivers/usb/gadget/f_fs.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index fffda61..e84b73b 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -587,7 +587,7 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file-private_data;
- struct usb_gadget *gadget = epfile-ffs-gadget;
+ struct usb_gadget *gadget;
struct ffs_ep *ep;
char *data = NULL;
ssize_t ret, data_len;
@@ -613,6 +613,11 @@ static ssize_t ffs_epfile_io(struct file *file,
goto error;
}
}
+ /*
+* if we _do_ wait above, the epfile-ffs-gadget might be NULL
+* before the waiting completes, so do not assign to 'gadget' earlier
+*/
+ gadget = epfile-ffs-gadget;
/* Do we halt? */
halt = !read == !epfile-in;
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
