Re: [PATCH] net: Fix potential NULL pointer dereference in __skb_try_recv_datagram

[Rainer Weikusat]

Jacob Siverskog <jacob@xxxxxxxxxxxxxxxxxxx> writes:
> On Tue, Dec 29, 2015 at 9:08 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:
>> From: Rainer Weikusat <rweikusat@xxxxxxxxxxxxxxxxxxxxxxx>
>> Date: Tue, 29 Dec 2015 19:42:36 +0000
>>
>>> Jacob Siverskog <jacob@xxxxxxxxxxxxxxxxxxx> writes:
>>>> This should fix a NULL pointer dereference I encountered (dump
>>>> below). Since __skb_unlink is called while walking,
>>>> skb_queue_walk_safe should be used.
>>>
>>> The code in question is:
>>  ...
>>> __skb_unlink is only called prior to returning from the function.
>>> Consequently, it won't affect the skb_queue_walk code.
>>
>> Agreed, this patch doesn't fix anything.
>
> Ok. Thanks for your feedback. How do you believe the issue could be
> solved? Investigating it gives:
>
> static inline void __skb_unlink(struct sk_buff *skb, struct sk_buff_head *list)
> {

[...]

> next->prev = prev;
>      530: e5823004 str r3, [r2, #4]                          <--
> trapping instruction (r2 NULL)
>
> Register contents:
> r7 : c58cfe1c  r6 : c06351d0  r5 : c77810ac  r4 : c583eac0
> r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : 20000013
>
> If I understand this correctly, then r4 = skb, r2 = next, r3 = prev.

Some additional information which may be helpful: The next->prev = prev
was pretty obvious from the original error message alone: The invalid
access happened at 4 but no register contained 4. Considering that this
is for ARM, this must have been caused by an instruction using an
address of the form

[Rx, #4]

ie, value of register x + 4. And the next->prev = prev is the only
access to something located 4 bytes beyond something else.

> Should there be a check for this in __skb_try_recv_datagram?

These lists are supposed to be circular, ie, the next pointer of the
last element should point to the first and the prev pointer of the first
to the last. If there's an element with ->next == NULL on the list,
something either didn't do inserts correctly or corrupted an originally
intact list.

General advice: The original error occurred with 4.3.0. Had this
happened to me, I'd either tried to locate the error in the same kernel
version or to reproduce the bug with the one I was planning to
modify. Trying to fix a 'strange memory access' error which was observed
with version x.y by modifying version x.z is IMHO needlessly moving on
shaky ground.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/