linux-kernel - Re: [linux-usb-devel] [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0707301053020.3113@bender.nucleusys.com>
Date:	Mon, 30 Jul 2007 11:05:25 +0300 (EEST)
From:	Petko Manolov <petkan@...leusys.com>
To:	Oliver Neukum <oliver@...kum.org>
cc:	linux-usb-devel@...ts.sourceforge.net,
	Jesper Juhl <jesper.juhl@...il.com>,
	Satyam Sharma <satyam.sharma@...il.com>,
	Greg Kroah-Hartman <greg@...ah.com>, netdev@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Petko Manolov <petkan@...rs.sourceforge.net>
Subject: 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 <satyam.sharma@...il.com> wrote:
>>> Hi,
>>>
>>> On 7/29/07, Jesper Juhl <jesper.juhl@...il.com> 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
View attachment "pegasus.patch" of type "TEXT/x-diff" (1403 bytes)