Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
On Mon, 30 Jul 2007, Petko Manolov wrote: > On Sun, 29 Jul 2007, Oliver Neukum wrote: > > > [...] > > pegasus == NULL there would be a kernel bug. Silently ignoring > > it, like the code now wants to do is bad. As the oops has never been > > reported, I figure turning it into an explicit debugging test is overkill, > > so removal seems to be the best option. > > In the past urb->context was not guaranteed to be non-null for any > asynchronous calls. If this is not the case anymore then it should be removed > from at least two more locations in the driver. > > Attached you'll find the resulting patch. Given Oliver's earlier comment, it looks okay to me. Thanks. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
On Sun, 29 Jul 2007, Oliver Neukum wrote:
Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:
On 29/07/07, Satyam Sharma <[EMAIL PROTECTED]> wrote:
Hi,
On 7/29/07, Jesper Juhl <[EMAIL PROTECTED]> wrote:
Hello,
This patch makes sure we don't dereference a NULL pointer in
drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
struct net_device *net = pegasus->net; assignment.
The existing code checks if 'pegasus' is NULL and bails out if
it is, so we better not touch that pointer until after that check.
[...]
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..04cba6b 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -768,11 +768,13 @@ done:
static void write_bulk_callback(struct urb *urb)
{
pegasus_t *pegasus = urb->context;
- struct net_device *net = pegasus->net;
+ struct net_device *net;
if (!pegasus)
return;
+ net = pegasus->net;
+
if (!netif_device_present(net) || !netif_running(net))
return;
Is it really possible that we're called into this function with
urb->context == NULL? If not, I'd suggest let's just get rid of
the check itself, instead.
I'm not sure. I am not very familiar with this code. I just figured
that moving the assignment is potentially a little safer and it is
certainly no worse than the current code, so that's a safe and
potentially benneficial change. Removing the check may be safe but I'm
not certain enough to make that call...
pegasus == NULL there would be a kernel bug. Silently ignoring
it, like the code now wants to do is bad. As the oops has never been
reported, I figure turning it into an explicit debugging test is overkill,
so removal seems to be the best option.
In the past urb->context was not guaranteed to be non-null for any
asynchronous calls. If this is not the case anymore then it should be
removed from at least two more locations in the driver.
Attached you'll find the resulting patch.
Petko--- drivers/net/usb/pegasus.c.old 2007-07-10 11:39:50.0 +0300
+++ drivers/net/usb/pegasus.c 2007-07-30 11:02:10.0 +0300
@@ -100,9 +100,6 @@ static void ctrl_callback(struct urb *ur
{
pegasus_t *pegasus = urb->context;
- if (!pegasus)
- return;
-
switch (urb->status) {
case 0:
if (pegasus->flags & ETH_REGS_CHANGE) {
@@ -609,15 +606,11 @@ static inline struct sk_buff *pull_skb(p
static void read_bulk_callback(struct urb *urb)
{
pegasus_t *pegasus = urb->context;
- struct net_device *net;
+ struct net_device *net = pegasus->net;
int rx_status, count = urb->actual_length;
u8 *buf = urb->transfer_buffer;
__u16 pkt_len;
- if (!pegasus)
- return;
-
- net = pegasus->net;
if (!netif_device_present(net) || !netif_running(net))
return;
@@ -770,9 +763,6 @@ static void write_bulk_callback(struct u
pegasus_t *pegasus = urb->context;
struct net_device *net = pegasus->net;
- if (!pegasus)
- return;
-
if (!netif_device_present(net) || !netif_running(net))
return;
@@ -805,13 +795,9 @@ static void write_bulk_callback(struct u
static void intr_callback(struct urb *urb)
{
pegasus_t *pegasus = urb->context;
- struct net_device *net;
+ struct net_device *net = pegasus->net;
int status;
- if (!pegasus)
- return;
- net = pegasus->net;
-
switch (urb->status) {
case 0:
break;
Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
On Sun, 29 Jul 2007, Oliver Neukum wrote:
Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:
On 29/07/07, Satyam Sharma [EMAIL PROTECTED] wrote:
Hi,
On 7/29/07, Jesper Juhl [EMAIL PROTECTED] wrote:
Hello,
This patch makes sure we don't dereference a NULL pointer in
drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
struct net_device *net = pegasus-net; assignment.
The existing code checks if 'pegasus' is NULL and bails out if
it is, so we better not touch that pointer until after that check.
[...]
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..04cba6b 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -768,11 +768,13 @@ done:
static void write_bulk_callback(struct urb *urb)
{
pegasus_t *pegasus = urb-context;
- struct net_device *net = pegasus-net;
+ struct net_device *net;
if (!pegasus)
return;
+ net = pegasus-net;
+
if (!netif_device_present(net) || !netif_running(net))
return;
Is it really possible that we're called into this function with
urb-context == NULL? If not, I'd suggest let's just get rid of
the check itself, instead.
I'm not sure. I am not very familiar with this code. I just figured
that moving the assignment is potentially a little safer and it is
certainly no worse than the current code, so that's a safe and
potentially benneficial change. Removing the check may be safe but I'm
not certain enough to make that call...
pegasus == NULL there would be a kernel bug. Silently ignoring
it, like the code now wants to do is bad. As the oops has never been
reported, I figure turning it into an explicit debugging test is overkill,
so removal seems to be the best option.
In the past urb-context was not guaranteed to be non-null for any
asynchronous calls. If this is not the case anymore then it should be
removed from at least two more locations in the driver.
Attached you'll find the resulting patch.
Petko--- drivers/net/usb/pegasus.c.old 2007-07-10 11:39:50.0 +0300
+++ drivers/net/usb/pegasus.c 2007-07-30 11:02:10.0 +0300
@@ -100,9 +100,6 @@ static void ctrl_callback(struct urb *ur
{
pegasus_t *pegasus = urb-context;
- if (!pegasus)
- return;
-
switch (urb-status) {
case 0:
if (pegasus-flags ETH_REGS_CHANGE) {
@@ -609,15 +606,11 @@ static inline struct sk_buff *pull_skb(p
static void read_bulk_callback(struct urb *urb)
{
pegasus_t *pegasus = urb-context;
- struct net_device *net;
+ struct net_device *net = pegasus-net;
int rx_status, count = urb-actual_length;
u8 *buf = urb-transfer_buffer;
__u16 pkt_len;
- if (!pegasus)
- return;
-
- net = pegasus-net;
if (!netif_device_present(net) || !netif_running(net))
return;
@@ -770,9 +763,6 @@ static void write_bulk_callback(struct u
pegasus_t *pegasus = urb-context;
struct net_device *net = pegasus-net;
- if (!pegasus)
- return;
-
if (!netif_device_present(net) || !netif_running(net))
return;
@@ -805,13 +795,9 @@ static void write_bulk_callback(struct u
static void intr_callback(struct urb *urb)
{
pegasus_t *pegasus = urb-context;
- struct net_device *net;
+ struct net_device *net = pegasus-net;
int status;
- if (!pegasus)
- return;
- net = pegasus-net;
-
switch (urb-status) {
case 0:
break;
Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
On Mon, 30 Jul 2007, Petko Manolov wrote: On Sun, 29 Jul 2007, Oliver Neukum wrote: [...] pegasus == NULL there would be a kernel bug. Silently ignoring it, like the code now wants to do is bad. As the oops has never been reported, I figure turning it into an explicit debugging test is overkill, so removal seems to be the best option. In the past urb-context was not guaranteed to be non-null for any asynchronous calls. If this is not the case anymore then it should be removed from at least two more locations in the driver. Attached you'll find the resulting patch. Given Oliver's earlier comment, it looks okay to me. Thanks. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
On Sun, 29 Jul 2007, Oliver Neukum wrote:
> Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:
> > On 29/07/07, Satyam Sharma <[EMAIL PROTECTED]> wrote:
> > > Hi,
> > >
> > > On 7/29/07, Jesper Juhl <[EMAIL PROTECTED]> wrote:
> > > > Hello,
> > > >
> > > > This patch makes sure we don't dereference a NULL pointer in
> > > > drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
> > > > struct net_device *net = pegasus->net; assignment.
> > > > The existing code checks if 'pegasus' is NULL and bails out if
> > > > it is, so we better not touch that pointer until after that check.
> > > > [...]
> > >
> > > Is it really possible that we're called into this function with
> > > urb->context == NULL? If not, I'd suggest let's just get rid of
> > > the check itself, instead.
> > >
> > I'm not sure. I am not very familiar with this code. I just figured
> > that moving the assignment is potentially a little safer and it is
> > certainly no worse than the current code, so that's a safe and
> > potentially benneficial change. Removing the check may be safe but I'm
> > not certain enough to make that call...
>
> pegasus == NULL there would be a kernel bug. Silently ignoring
> it, like the code now wants to do is bad. As the oops has never been
> reported, I figure turning it into an explicit debugging test is overkill,
> so removal seems to be the best option.
Ok, thanks. Updated patch below.
[PATCH] pegasus: Remove bogus checks in urb->complete() callbacks
urb->complete() callbacks registered in drivers/net/usb/pegasus.c needlessly
check for urb->context != NULL, but that is not possible (the only way that
can be possible would be a kernel bug elsewhere, and these checks would
simply end up hiding that). So let's remove the bogus checks.
Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>
---
drivers/net/usb/pegasus.c |9 +
1 files changed, 1 insertions(+), 8 deletions(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..439ef9f 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -770,9 +770,6 @@ static void write_bulk_callback(struct urb *urb)
pegasus_t *pegasus = urb->context;
struct net_device *net = pegasus->net;
- if (!pegasus)
- return;
-
if (!netif_device_present(net) || !netif_running(net))
return;
@@ -805,13 +802,9 @@ static void write_bulk_callback(struct urb *urb)
static void intr_callback(struct urb *urb)
{
pegasus_t *pegasus = urb->context;
- struct net_device *net;
+ struct net_device *net = pegasus->net;
int status;
- if (!pegasus)
- return;
- net = pegasus->net;
-
switch (urb->status) {
case 0:
break;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:
> On 29/07/07, Satyam Sharma <[EMAIL PROTECTED]> wrote:
> > Hi,
> >
> > On 7/29/07, Jesper Juhl <[EMAIL PROTECTED]> wrote:
> > > Hello,
> > >
> > > This patch makes sure we don't dereference a NULL pointer in
> > > drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
> > > struct net_device *net = pegasus->net; assignment.
> > > The existing code checks if 'pegasus' is NULL and bails out if
> > > it is, so we better not touch that pointer until after that check.
> > > [...]
> > > diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> > > index a05fd97..04cba6b 100644
> > > --- a/drivers/net/usb/pegasus.c
> > > +++ b/drivers/net/usb/pegasus.c
> > > @@ -768,11 +768,13 @@ done:
> > > static void write_bulk_callback(struct urb *urb)
> > > {
> > > pegasus_t *pegasus = urb->context;
> > > - struct net_device *net = pegasus->net;
> > > + struct net_device *net;
> > >
> > > if (!pegasus)
> > > return;
> > >
> > > + net = pegasus->net;
> > > +
> > > if (!netif_device_present(net) || !netif_running(net))
> > > return;
> >
> > Is it really possible that we're called into this function with
> > urb->context == NULL? If not, I'd suggest let's just get rid of
> > the check itself, instead.
> >
> I'm not sure. I am not very familiar with this code. I just figured
> that moving the assignment is potentially a little safer and it is
> certainly no worse than the current code, so that's a safe and
> potentially benneficial change. Removing the check may be safe but I'm
> not certain enough to make that call...
pegasus == NULL there would be a kernel bug. Silently ignoring
it, like the code now wants to do is bad. As the oops has never been
reported, I figure turning it into an explicit debugging test is overkill,
so removal seems to be the best option.
Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:
On 29/07/07, Satyam Sharma [EMAIL PROTECTED] wrote:
Hi,
On 7/29/07, Jesper Juhl [EMAIL PROTECTED] wrote:
Hello,
This patch makes sure we don't dereference a NULL pointer in
drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
struct net_device *net = pegasus-net; assignment.
The existing code checks if 'pegasus' is NULL and bails out if
it is, so we better not touch that pointer until after that check.
[...]
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..04cba6b 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -768,11 +768,13 @@ done:
static void write_bulk_callback(struct urb *urb)
{
pegasus_t *pegasus = urb-context;
- struct net_device *net = pegasus-net;
+ struct net_device *net;
if (!pegasus)
return;
+ net = pegasus-net;
+
if (!netif_device_present(net) || !netif_running(net))
return;
Is it really possible that we're called into this function with
urb-context == NULL? If not, I'd suggest let's just get rid of
the check itself, instead.
I'm not sure. I am not very familiar with this code. I just figured
that moving the assignment is potentially a little safer and it is
certainly no worse than the current code, so that's a safe and
potentially benneficial change. Removing the check may be safe but I'm
not certain enough to make that call...
pegasus == NULL there would be a kernel bug. Silently ignoring
it, like the code now wants to do is bad. As the oops has never been
reported, I figure turning it into an explicit debugging test is overkill,
so removal seems to be the best option.
Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
On Sun, 29 Jul 2007, Oliver Neukum wrote:
Am Sonntag 29 Juli 2007 schrieb Jesper Juhl:
On 29/07/07, Satyam Sharma [EMAIL PROTECTED] wrote:
Hi,
On 7/29/07, Jesper Juhl [EMAIL PROTECTED] wrote:
Hello,
This patch makes sure we don't dereference a NULL pointer in
drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
struct net_device *net = pegasus-net; assignment.
The existing code checks if 'pegasus' is NULL and bails out if
it is, so we better not touch that pointer until after that check.
[...]
Is it really possible that we're called into this function with
urb-context == NULL? If not, I'd suggest let's just get rid of
the check itself, instead.
I'm not sure. I am not very familiar with this code. I just figured
that moving the assignment is potentially a little safer and it is
certainly no worse than the current code, so that's a safe and
potentially benneficial change. Removing the check may be safe but I'm
not certain enough to make that call...
pegasus == NULL there would be a kernel bug. Silently ignoring
it, like the code now wants to do is bad. As the oops has never been
reported, I figure turning it into an explicit debugging test is overkill,
so removal seems to be the best option.
Ok, thanks. Updated patch below.
[PATCH] pegasus: Remove bogus checks in urb-complete() callbacks
urb-complete() callbacks registered in drivers/net/usb/pegasus.c needlessly
check for urb-context != NULL, but that is not possible (the only way that
can be possible would be a kernel bug elsewhere, and these checks would
simply end up hiding that). So let's remove the bogus checks.
Signed-off-by: Satyam Sharma [EMAIL PROTECTED]
---
drivers/net/usb/pegasus.c |9 +
1 files changed, 1 insertions(+), 8 deletions(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..439ef9f 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -770,9 +770,6 @@ static void write_bulk_callback(struct urb *urb)
pegasus_t *pegasus = urb-context;
struct net_device *net = pegasus-net;
- if (!pegasus)
- return;
-
if (!netif_device_present(net) || !netif_running(net))
return;
@@ -805,13 +802,9 @@ static void write_bulk_callback(struct urb *urb)
static void intr_callback(struct urb *urb)
{
pegasus_t *pegasus = urb-context;
- struct net_device *net;
+ struct net_device *net = pegasus-net;
int status;
- if (!pegasus)
- return;
- net = pegasus-net;
-
switch (urb-status) {
case 0:
break;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
