From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752552AbbJFHV4 (ORCPT ); Tue, 6 Oct 2015 03:21:56 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:33743 "EHLO mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751573AbbJFHVz (ORCPT ); Tue, 6 Oct 2015 03:21:55 -0400 Date: Tue, 6 Oct 2015 09:21:51 +0200 From: Ingo Molnar To: Chris Metcalf Cc: Linus Torvalds , Alexey Dobriyan , Linux Kernel Mailing List , Peter Zijlstra , Thomas Gleixner , "H. Peter Anvin" , Borislav Petkov Subject: Re: [PATCH] string: Fix strscpy() uninitialized data copy bug Message-ID: <20151006072151.GA10672@gmail.com> References: <20151005162226.GA10993@gmail.com> <20151005162802.GA11474@gmail.com> <20151005163641.GA11635@gmail.com> <5612C749.8090308@ezchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5612C749.8090308@ezchip.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Chris Metcalf wrote: > Unfortunately using memset() like that will break on big-endian machines. doh ... and I somehow convinced myself that it was endian safe ;-) > [...] I always have to go back and play around with the word-at-a-time.h > definitions to get this right, but I think it's possible that the "data" itself > has the mask to clear the unwanted bytes, i.e. you could do something like the > following (untested). > > I'm still not totally convinced it's necessary, as programmers should generally > assume anything beyond the end of a copied string is garbage anyway, and since > we're not copying it to userspace we're not exposing any possibly secure data. > > Races shouldn't be a concern either since, after all, there is already a window > where we may have overwritten the NUL end of an earlier shorter string, and now > a racy copy from the partially-written dest buf could walk right off the end of > the buffer itself, so you'd already better not be doing that. > > But, all that said, I'm not opposed to a simple fix to avoid carrying along the > uninitialized bytes from beyond the end of the source string, since it does seem > a bit cleaner, even if I can't put my finger in a reason why it would actually > matter. So it would matter for more advanced sharing ABIs: for example if there's an mlock()-ed area registered on the kernel side as well as kernel accessible memory, and if we do an strscpy() to such a target area, we don't want to leak uninitialized data to user-space. (This is not theoretical, the perf ring-buffer is such a construct for example.) So IMHO this is a quality of implementation issue that we should fix. > diff --git a/lib/string.c b/lib/string.c > index 8dbb7b1eab50..ba64f4e0382d 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -203,12 +203,13 @@ ssize_t strscpy(char *dest, const char *src, size_t count) > unsigned long c, data; > c = *(unsigned long *)(src+res); > - *(unsigned long *)(dest+res) = c; > if (has_zero(c, &data, &constants)) { > data = prep_zero_mask(c, data, &constants); > data = create_zero_mask(data); > + *(unsigned long *)(dest+res) = c & data; > return res + find_zero(data); > } > + *(unsigned long *)(dest+res) = c; > res += sizeof(unsigned long); > count -= sizeof(unsigned long); > max -= sizeof(unsigned long); Looks good to me! Thanks, Ingo