Messages in this thread Patch in this message | | | Subject | Re: [PATCH] string: Fix strscpy() uninitialized data copy bug | From | Chris Metcalf <> | Date | Mon, 5 Oct 2015 14:54:01 -0400 |
| |
On 10/05/2015 12:36 PM, Ingo Molnar wrote: > So the patch below got tested a bit more seriously, with the strscpy() based > strlcpy() patch I sent earlier: at least a typical Fedora bootup with a few > thousand strlcpy() uses does not crash in any obvious way. > > Still needs review to make sure I have not missed anything ... > > Thanks, > > Ingo > > ===================> > From 946bab4d7138e5db53c5f1759e97809ebdf89551 Mon Sep 17 00:00:00 2001 > From: Ingo Molnar<mingo@kernel.org> > Date: Mon, 5 Oct 2015 18:30:37 +0200 > Subject: [PATCH] string: Fix strscpy() uninitialized data copy bug > > Alexey Dobriyan noticed that our new strscpy() implementation will copy > potentially out of range or uninitialized data from post the end of the > source string. > > Fix this by zeroing out the tail of the final word of the copy. > > Reported-by: Alexey Dobriyan<adobriyan@gmail.com> > Signed-off-by: Ingo Molnar<mingo@kernel.org> > --- > lib/string.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/lib/string.c b/lib/string.c > index 6b89c035df74..548f52b7a145 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -177,12 +177,24 @@ 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)) { > + unsigned int zero_pos; > + > data = prep_zero_mask(c, data, &constants); > data = create_zero_mask(data); > + > + zero_pos = find_zero(data); > + > + /* Clear out undefined data within the final word after the NUL (if any): */ > + memset((void *)&c + zero_pos, 0, sizeof(long)-zero_pos);
Unfortunately using memset() like that will break on big-endian machines. 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.
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); -- Chris Metcalf, EZChip Semiconductor http://www.ezchip.com
| |