[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#639919: linux-2.6: please enable DEBUG_STRICT_USER_COPY_CHECKS



On Thu, 2011-09-01 at 08:17 +0200, Yves-Alexis Perez wrote:
> On jeu., 2011-09-01 at 05:21 +0100, Ben Hutchings wrote:
> > As I wrote on #605090:
> > 
> > Without the strict check, the crap code produces a compile-time warning
> > and a run-time warning and *no copying*.  With the strict check, the
> > crap code results in FTBFS (but only on i386 and s390!).  So how is this
> > an improvement for us?
> 
> That mean we can catch issues earlier, at least on those arches (and if
> they trigger on theses arches that surely means they exist on the
> others). 
> 
> The warning is good, but are those really seen in the huge build log?
> Maybe all the relevant crap code is detected before it reaches debian
> buildd and thus we don't need the FTBFS, but I'm still unsure.
> 
> Or maybe just enable it for trunk/-rc packages and not in sid/ builds (I
> can understand that FTBFS are a pain but in those cases I'm not sure
> warnings are sufficient).

Here's why we really can't do this:

In file included from .../arch/x86/include/asm/uaccess.h:573:0,
                 from .../include/linux/uaccess.h:5,
                 from .../include/linux/highmem.h:7,
                 from .../include/linux/pagemap.h:10,
                 from .../fs/binfmt_misc.c:26:
.../include/asm/uaccess_32.h: In function 'parse_command.part.4':
.../arch/x86/include/asm/uaccess_32.h:211:26: warning: call to 'copy_from_user_overflow' declared with attribute warning: copy_from_user() buffer size is not provably correct [enabled by default]

Now look at the code it's complaining about:

static int parse_command(const char __user *buffer, size_t count)
{
	char s[4];

	if (!count)
		return 0;
	if (count > 3)
		return -EINVAL;
	if (copy_from_user(s, buffer, count))
		return -EFAULT;
	if (s[count-1] == '\n')
		count--;
	if (count == 1 && s[0] == '0')
		return 1;
	if (count == 1 && s[0] == '1')
		return 2;
	if (count == 2 && s[0] == '-' && s[1] == '1')
		return 3;
	return -EINVAL;
}

So sizeof(s) == 4 and count <= 3, but the compiler is still too stupid
to avoid generating a conditional call to copy_from_user_overflow().
And this would break the build if we did what you're asking.

Ben.

-- 
Ben Hutchings
Knowledge is power.  France is bacon.

Attachment: signature.asc
Description: This is a digitally signed message part


Reply to: