lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 6 Oct 2015 09:21:51 +0200 From: Ingo Molnar <mingo@...nel.org> To: Chris Metcalf <cmetcalf@...hip.com> Cc: Linus Torvalds <torvalds@...ux-foundation.org>, Alexey Dobriyan <adobriyan@...il.com>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Peter Zijlstra <a.p.zijlstra@...llo.nl>, Thomas Gleixner <tglx@...utronix.de>, "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de> Subject: Re: [PATCH] string: Fix strscpy() uninitialized data copy bug * Chris Metcalf <cmetcalf@...hip.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists