Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754612AbbL3Pcv (ORCPT ); Wed, 30 Dec 2015 10:32:51 -0500 Received: from tiger.mobileactivedefense.com ([217.174.251.109]:55736 "EHLO tiger.mobileactivedefense.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751506AbbL3Pct (ORCPT ); Wed, 30 Dec 2015 10:32:49 -0500 From: Rainer Weikusat To: Jacob Siverskog Cc: David Miller , Rainer Weikusat , netdev@vger.kernel.org, Herbert Xu , Eric Dumazet , Konstantin Khlebnikov , Al Viro , LKML Subject: Re: [PATCH] net: Fix potential NULL pointer dereference in __skb_try_recv_datagram In-Reply-To: (Jacob Siverskog's message of "Wed, 30 Dec 2015 12:14:42 +0100") References: <1451416224-15871-1-git-send-email-jacob@teenage.engineering> <87y4cdyrbn.fsf@doppelsaurus.mobileactivedefense.com> <20151229.150843.2021692616139434395.davem@davemloft.net> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux) Date: Wed, 30 Dec 2015 15:32:31 +0000 Message-ID: <87fuykuf3k.fsf@doppelsaurus.mobileactivedefense.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (tiger.mobileactivedefense.com [217.174.251.109]); Wed, 30 Dec 2015 15:32:38 +0000 (GMT) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2617 Lines: 66 Jacob Siverskog writes: > On Tue, Dec 29, 2015 at 9:08 PM, David Miller wrote: >> From: Rainer Weikusat >> Date: Tue, 29 Dec 2015 19:42:36 +0000 >> >>> Jacob Siverskog 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/