* [usb-host] question about pointer dereference before null check
@ 2017-05-01 23:35 Gustavo A. R. Silva
2017-05-02 14:19 ` Alan Stern
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-01 23:35 UTC (permalink / raw)
To: Greg Kroah-Hartman, Alan Stern
Cc: linux-usb, linux-kernel, Peter Senna Tschudin
Hello everybody,
While taking a look into Coverity ID 100828 I ran into the following
piece of code at drivers/usb/host/ehci-sched.c:1096:
u32 addr;
int think_time;
int hs_transfers;
addr = dev->ttport << 24;
if (!ehci_is_TDI(ehci)
|| (dev->tt->hub !=
ehci_to_hcd(ehci)->self.root_hub))
addr |= dev->tt->hub->devnum << 16;
addr |= epnum << 8;
addr |= dev->devnum;
stream->ps.usecs = HS_USECS_ISO(maxp);
think_time = dev->tt ? dev->tt->think_time : 0;
The issue here is that dev->tt is being dereferenced before null check.
I was thinking on placing think_time = dev->tt ? dev->tt->think_time :
0; just before the _if_ statement. But this doesn't address the
problem of dev->tt actually being NULL.
While looking into the callers of the function containing this piece
of code (iso_stream_init()) my impression is that dev->tt is not NULL
at the time this function is called and, a very simple patch like the
following can be applied in order to avoid the Coverity issue:
-think_time = dev->tt ? dev->tt->think_time : 0;
+think_time = dev->tt->think_time;
But I can't tell for sure, so in case dev->tt is NULL, a good strategy
to properly update the _addr_ variable would be needed.
What do you think?
I would really appreciate any comment on this,
Thank you!
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [usb-host] question about pointer dereference before null check
2017-05-01 23:35 [usb-host] question about pointer dereference before null check Gustavo A. R. Silva
@ 2017-05-02 14:19 ` Alan Stern
2017-05-02 15:13 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Alan Stern @ 2017-05-02 14:19 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Senna Tschudin
On Mon, 1 May 2017, Gustavo A. R. Silva wrote:
> Hello everybody,
>
> While taking a look into Coverity ID 100828 I ran into the following
> piece of code at drivers/usb/host/ehci-sched.c:1096:
>
> u32 addr;
> int think_time;
> int hs_transfers;
>
> addr = dev->ttport << 24;
> if (!ehci_is_TDI(ehci)
> || (dev->tt->hub !=
> ehci_to_hcd(ehci)->self.root_hub))
> addr |= dev->tt->hub->devnum << 16;
> addr |= epnum << 8;
> addr |= dev->devnum;
> stream->ps.usecs = HS_USECS_ISO(maxp);
> think_time = dev->tt ? dev->tt->think_time : 0;
>
> The issue here is that dev->tt is being dereferenced before null check.
>
> I was thinking on placing think_time = dev->tt ? dev->tt->think_time :
> 0; just before the _if_ statement. But this doesn't address the
> problem of dev->tt actually being NULL.
>
> While looking into the callers of the function containing this piece
> of code (iso_stream_init()) my impression is that dev->tt is not NULL
> at the time this function is called and, a very simple patch like the
> following can be applied in order to avoid the Coverity issue:
>
> -think_time = dev->tt ? dev->tt->think_time : 0;
> +think_time = dev->tt->think_time;
>
> But I can't tell for sure, so in case dev->tt is NULL, a good strategy
> to properly update the _addr_ variable would be needed.
>
> What do you think?
>
> I would really appreciate any comment on this,
> Thank you!
You are right; udev->tt cannot ever be NULL when this section of code
runs. The test should be removed.
Alan Stern
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [usb-host] question about pointer dereference before null check
2017-05-02 14:19 ` Alan Stern
@ 2017-05-02 15:13 ` Gustavo A. R. Silva
0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-02 15:13 UTC (permalink / raw)
To: Alan Stern
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Senna Tschudin
Hi Alan,
Quoting Alan Stern <stern@rowland.harvard.edu>:
> On Mon, 1 May 2017, Gustavo A. R. Silva wrote:
>
>> Hello everybody,
>>
>> While taking a look into Coverity ID 100828 I ran into the following
>> piece of code at drivers/usb/host/ehci-sched.c:1096:
>>
>> u32 addr;
>> int think_time;
>> int hs_transfers;
>>
>> addr = dev->ttport << 24;
>> if (!ehci_is_TDI(ehci)
>> || (dev->tt->hub !=
>> ehci_to_hcd(ehci)->self.root_hub))
>> addr |= dev->tt->hub->devnum << 16;
>> addr |= epnum << 8;
>> addr |= dev->devnum;
>> stream->ps.usecs = HS_USECS_ISO(maxp);
>> think_time = dev->tt ? dev->tt->think_time : 0;
>>
>> The issue here is that dev->tt is being dereferenced before null check.
>>
>> I was thinking on placing think_time = dev->tt ? dev->tt->think_time :
>> 0; just before the _if_ statement. But this doesn't address the
>> problem of dev->tt actually being NULL.
>>
>> While looking into the callers of the function containing this piece
>> of code (iso_stream_init()) my impression is that dev->tt is not NULL
>> at the time this function is called and, a very simple patch like the
>> following can be applied in order to avoid the Coverity issue:
>>
>> -think_time = dev->tt ? dev->tt->think_time : 0;
>> +think_time = dev->tt->think_time;
>>
>> But I can't tell for sure, so in case dev->tt is NULL, a good strategy
>> to properly update the _addr_ variable would be needed.
>>
>> What do you think?
>>
>> I would really appreciate any comment on this,
>> Thank you!
>
> You are right; udev->tt cannot ever be NULL when this section of code
> runs. The test should be removed.
>
Thanks for confirming, I'll send a patch shortly.
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-05-02 16:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-01 23:35 [usb-host] question about pointer dereference before null check Gustavo A. R. Silva
2017-05-02 14:19 ` Alan Stern
2017-05-02 15:13 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.