On Mon, Nov 26, 2012 at 08:26:11PM +0000, Markos Chandras wrote:
> On 26 November 2012 19:49, Rich Felker <dal...@aerifal.cx> wrote:
> > On Mon, Nov 26, 2012 at 02:31:06PM -0500, Mark Salter wrote:
> >> On Mon, 2012-11-26 at 14:24 +0000, Markos Chandras wrote:
> >> > +int __libc_statfs(const char *path, struct statfs *buf)
> >> > +{
> >> > + struct statfs64 b;
> >> > + int err;
> >> > +
> >> > + /*
> >> > + * See if pointer has a sane value.
> >> > + * This does not prevent the user from
> >> > + * passing an arbitrary possitive value
> >> > + * that can lead to a segfault or potential
> >> > + * security problems
> >> > + */
> >> > +
> >> > + if (buf == NULL || (int)buf < 0) {
> >> > + __set_errno(EFAULT);
> >> > + return -1;
> >> > + }
> >>
> >> This seems wrong. Doesn't the kernel already validate addresses passed
> >> in from userspace. Even in the no-MMU case, some architectures add
> >> basic checking for user addresses.
> >>
> >> In any case, the "(int)buf < 0" is clearly non-portable. C6X can have
> >> perfectly good addresses which make negative ints.
> >
> > Indeed, this code is definitely wrong as-written. There's no
> > obligation to fail with EFAULT when a bad address is given, so just
> > crashing is perfectly acceptable.
> >
> > With that said, I question why this code is even needed. If an arch
> > doesn't have a non-64-bit statfs call, then why would it have a
> > separate non-64-bit, legacy statfs structure? If it really needs one,
> > the statfs structure could just be defined to match the layout of the
> > statfs64 structure, but with 32-bit off_t fields and 32 bits of
> > padding next to them instead of the usual 64-bit off_t. Then no
> > conversion code would be needed.
> >
> > Rich
>
> This is just for backwards compatibility. This patch tries to use the
> existing structures so we
> don't have to avoid a new one for these architectures.
As far as I know, there's nothing to maintain "backwards
compatibility" with. The stat structures vary per-arch, and an arch
that doesn't have a non-64-bit statfs syscall doesn't have an existing
"struct statfs" to maintain backwards compatibility with. You can just
define "struct statfs" to match the layout of the 64-bit struct.
By the way, the old small-off_t functions are supposed to give an
error if any off_t value does not fit in the range of off_t, rather
than silently truncating. So even if you want to keep the conversion
code, you need to check this. If using my approach (with same struct
layout and padding for the extra 32 bits), you'd need to test that the
padding is zero and return an error if any nonzero value appeared in
the padding (except perhaps in the case of -1?).
> As for the simple "buf" pointer check, I think you missed the point.
> This pointer is never passed to the kernel
> (it is not passed to the INLINE_SYSCALL macro) so the kernel does not
> know about it. It is possible your userspace
> program to pass an invalid "buf" pointer and the INLINE_SYSCALL to
> succeed but you will crash after that when you try
> to dereference any member of the invalid "buf" pointer.
Crashing is perfectly acceptable behavior. statfs() is under no
obligation to report EFAULT; passing an invalid pointer is undefined
behavior and the expected result should be a crash or worse. Even if
the kernel handles it, passing bogus pointers won't necessarily give
EFAULT; if they happen to point to writable memory, you'll just
clobber random memory.
Rich
_______________________________________________
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc
