* re: net: dsa: hellcreek: Add support for hardware timestamping
@ 2020-11-09 13:38 Colin Ian King
2020-11-09 13:59 ` Kurt Kanzenbach
0 siblings, 1 reply; 3+ messages in thread
From: Colin Ian King @ 2020-11-09 13:38 UTC (permalink / raw)
To: Kamil Alkhouri, Kurt Kanzenbach, Richard Cochran, Jakub Kicinski
Cc: Andrew Lunn, ivien Didelot, Florian Fainelli, Vladimir Oltean,
netdev, linux-kernel, kernel-janitors
Hi
Static analysis on linux-next with Coverity has detected a potential
null pointer dereference issue on the following commit:
commit f0d4ba9eff75a79fccb7793f4d9f12303d458603
Author: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
Date: Tue Nov 3 08:10:58 2020 +0100
net: dsa: hellcreek: Add support for hardware timestamping
The analysis is as follows:
323 /* Get nanoseconds from ptp packet */
324 type = SKB_PTP_TYPE(skb);
4. returned_null: ptp_parse_header returns NULL (checked 10 out of 12
times).
5. var_assigned: Assigning: hdr = NULL return value from
ptp_parse_header.
325 hdr = ptp_parse_header(skb, type);
Dereference null return value (NULL_RETURNS)
6. dereference: Dereferencing a pointer that might be NULL hdr when
calling hellcreek_get_reserved_field.
326 ns = hellcreek_get_reserved_field(hdr);
327 hellcreek_clear_reserved_field(hdr);
This issue can only occur if the type & PTP_CLASS_PMASK is not one of
PTP_CLASS_IPV4, PTP_CLASS_IPV6 or PTP_CLASS_L2. I'm not sure if this is
a possibility or not, but I'm assuming that it would be useful to
perform the null check just in case, but I'm not sure how this affects
the hw timestamping code in this function.
Colin
^ permalink raw reply [flat|nested] 3+ messages in thread* re: net: dsa: hellcreek: Add support for hardware timestamping
2020-11-09 13:38 net: dsa: hellcreek: Add support for hardware timestamping Colin Ian King
@ 2020-11-09 13:59 ` Kurt Kanzenbach
2020-11-09 14:13 ` Colin Ian King
0 siblings, 1 reply; 3+ messages in thread
From: Kurt Kanzenbach @ 2020-11-09 13:59 UTC (permalink / raw)
To: Colin Ian King, Kamil Alkhouri, Richard Cochran, Jakub Kicinski
Cc: Andrew Lunn, ivien Didelot, Florian Fainelli, Vladimir Oltean,
netdev, linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]
Hi Colin,
On Mon Nov 09 2020, Colin Ian King wrote:
> Hi
>
> Static analysis on linux-next with Coverity has detected a potential
> null pointer dereference issue on the following commit:
>
> commit f0d4ba9eff75a79fccb7793f4d9f12303d458603
> Author: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
> Date: Tue Nov 3 08:10:58 2020 +0100
>
> net: dsa: hellcreek: Add support for hardware timestamping
>
> The analysis is as follows:
>
> 323 /* Get nanoseconds from ptp packet */
> 324 type = SKB_PTP_TYPE(skb);
>
> 4. returned_null: ptp_parse_header returns NULL (checked 10 out of 12
> times).
> 5. var_assigned: Assigning: hdr = NULL return value from
> ptp_parse_header.
>
> 325 hdr = ptp_parse_header(skb, type);
>
> Dereference null return value (NULL_RETURNS)
> 6. dereference: Dereferencing a pointer that might be NULL hdr when
> calling hellcreek_get_reserved_field.
>
> 326 ns = hellcreek_get_reserved_field(hdr);
> 327 hellcreek_clear_reserved_field(hdr);
>
> This issue can only occur if the type & PTP_CLASS_PMASK is not one of
> PTP_CLASS_IPV4, PTP_CLASS_IPV6 or PTP_CLASS_L2. I'm not sure if this is
> a possibility or not, but I'm assuming that it would be useful to
> perform the null check just in case, but I'm not sure how this affects
> the hw timestamping code in this function.
I don't see how the null pointer dereference could happen. That's the
Rx path you showed above.
The counter part code is:
hellcreek_port_rxtstamp:
/* Make sure the message is a PTP message that needs to be timestamped
* and the interaction with the HW timestamping is enabled. If not, stop
* here
*/
hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
if (!hdr)
return false;
SKB_PTP_TYPE(skb) = type;
Here the type is stored and hellcreek_should_tstamp() also calls
ptp_parse_header() internally. Only when ptp_parse_header() didn't
return NULL the first time the timestamping continues. It should be
safe.
Also the error handling would be interesting at that point. What should
happen if the header is null then? Returning an invalid timestamp?
Ignore it?
Hm. I think we have to make sure that it is a valid ptp packet before
reaching this code and that's what we've implemented. So, I guess it's
OK.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: net: dsa: hellcreek: Add support for hardware timestamping
2020-11-09 13:59 ` Kurt Kanzenbach
@ 2020-11-09 14:13 ` Colin Ian King
0 siblings, 0 replies; 3+ messages in thread
From: Colin Ian King @ 2020-11-09 14:13 UTC (permalink / raw)
To: Kurt Kanzenbach, Kamil Alkhouri, Richard Cochran, Jakub Kicinski
Cc: Andrew Lunn, ivien Didelot, Florian Fainelli, Vladimir Oltean,
netdev, linux-kernel, kernel-janitors
On 09/11/2020 13:59, Kurt Kanzenbach wrote:
> Hi Colin,
>
> On Mon Nov 09 2020, Colin Ian King wrote:
>> Hi
>>
>> Static analysis on linux-next with Coverity has detected a potential
>> null pointer dereference issue on the following commit:
>>
>> commit f0d4ba9eff75a79fccb7793f4d9f12303d458603
>> Author: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
>> Date: Tue Nov 3 08:10:58 2020 +0100
>>
>> net: dsa: hellcreek: Add support for hardware timestamping
>>
>> The analysis is as follows:
>>
>> 323 /* Get nanoseconds from ptp packet */
>> 324 type = SKB_PTP_TYPE(skb);
>>
>> 4. returned_null: ptp_parse_header returns NULL (checked 10 out of 12
>> times).
>> 5. var_assigned: Assigning: hdr = NULL return value from
>> ptp_parse_header.
>>
>> 325 hdr = ptp_parse_header(skb, type);
>>
>> Dereference null return value (NULL_RETURNS)
>> 6. dereference: Dereferencing a pointer that might be NULL hdr when
>> calling hellcreek_get_reserved_field.
>>
>> 326 ns = hellcreek_get_reserved_field(hdr);
>> 327 hellcreek_clear_reserved_field(hdr);
>>
>> This issue can only occur if the type & PTP_CLASS_PMASK is not one of
>> PTP_CLASS_IPV4, PTP_CLASS_IPV6 or PTP_CLASS_L2. I'm not sure if this is
>> a possibility or not, but I'm assuming that it would be useful to
>> perform the null check just in case, but I'm not sure how this affects
>> the hw timestamping code in this function.
>
> I don't see how the null pointer dereference could happen. That's the
> Rx path you showed above.
>
> The counter part code is:
>
> hellcreek_port_rxtstamp:
>
> /* Make sure the message is a PTP message that needs to be timestamped
> * and the interaction with the HW timestamping is enabled. If not, stop
> * here
> */
> hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
> if (!hdr)
> return false;
>
> SKB_PTP_TYPE(skb) = type;
>
> Here the type is stored and hellcreek_should_tstamp() also calls
> ptp_parse_header() internally. Only when ptp_parse_header() didn't
> return NULL the first time the timestamping continues. It should be
> safe.
>
> Also the error handling would be interesting at that point. What should
> happen if the header is null then? Returning an invalid timestamp?
> Ignore it?
>
> Hm. I think we have to make sure that it is a valid ptp packet before
> reaching this code and that's what we've implemented. So, I guess it's
> OK.
OK - thanks, I'll mark this as a false positive.
>
> Thanks,
> Kurt
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-09 14:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 13:38 net: dsa: hellcreek: Add support for hardware timestamping Colin Ian King
2020-11-09 13:59 ` Kurt Kanzenbach
2020-11-09 14:13 ` Colin Ian King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).