From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752895AbcDARjZ (ORCPT ); Fri, 1 Apr 2016 13:39:25 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:36686 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751281AbcDARjY (ORCPT ); Fri, 1 Apr 2016 13:39:24 -0400 Date: Fri, 1 Apr 2016 18:39:20 +0100 From: Al Viro To: Takashi Iwai Cc: Jiri Slaby , Andrew Morton , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Message-ID: <20160401173919.GC17997@ZenIV.linux.org.uk> References: <1459522924-17720-1-git-send-email-tiwai@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1459522924-17720-1-git-send-email-tiwai@suse.de> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 01, 2016 at 05:02:04PM +0200, Takashi Iwai wrote: > Currently, iov_iter_advance() just calls iterate_and_advance() macro > as is, even if size=0 is passed. Usually it is OK to pass size=0 to > the macro. However, when the iov_iter has been already advanced to > the end of the array, it may lead to an out-of-bound access, since the > macro always reads the length of the vector at first. This bug is > actually seen via KASAN with net tun driver, for example. FWIW, I think it's better dealt with in callers - almost all such cases are signs of bugs in the calling code and quietly hiding them is not going to fix the underlying bugs. > [] ? kasan_report_error+0x507/0x540 > [] ? __might_fault+0x3f/0x50 > [] ? __asan_report_load8_noabort+0x43/0x50 > [] ? iov_iter_advance+0x510/0x540 > [] ? iov_iter_advance+0x510/0x540 > [] ? tun_get_user+0x745/0x21a0 [tun] So tun_get_user() has a problem. > This patch adds the proper check of the size to iov_iter_advance(), > like all other functions calling iterate_and_advance() macro. NAK. If anything, turn that check into WARN_ON() to make sure it isn't missed. And tun_get_user() does seem to have a problem - I would like to see a reproducer, but it looks like some in the code that decides whether to use zerocopy mechanism and I'm not at all sure that this change (i.e. silently limit the amount we are advancing for) would end up doing the right thing.