* Chris Metcalf <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
