Suggestion on "int len" sanity

Suggestion on "int len" sanity

[XIAO Gang]

I would like to make a security suggestion.

There are many length variables in the kernel, locally declared as "len" 
or "length", either as "int", "unsigned int" or "size_t". However, 
declaring a length as "int" leads easily to an erroneous situation, as 
the author (or even a code checker) might make the implicite hypothesis 
that the length is positive, so that it is enough to make a sanity check 
of the kind

if (length > limit) ERROR;

which is not enough.

On the other hand, when a variable is named "len" or "length", it is 
usually used for length and never should go negative. So could I suggest 
that the declarations of these variables to be uniformized to "size_t", 
via a gradual but sysmatic cleanup?

-- 

XIAO Gang (~{P$8U~})                          xiao@unice.fr
          home page: pcmath126.unice.fr/xiao.html 

Re: Suggestion on "int len" sanity

[Jörn Engel]

On Wed, 1 June 2005 09:06:33 +0200, XIAO Gang wrote:
> 
> I would like to make a security suggestion.
> 
> There are many length variables in the kernel, locally declared as "len" 
> or "length", either as "int", "unsigned int" or "size_t". However, 
> declaring a length as "int" leads easily to an erroneous situation, as 
> the author (or even a code checker) might make the implicite hypothesis 
> that the length is positive, so that it is enough to make a sanity check 
> of the kind
> 
> if (length > limit) ERROR;
> 
> which is not enough.
> 
> On the other hand, when a variable is named "len" or "length", it is 
> usually used for length and never should go negative. So could I suggest 
> that the declarations of these variables to be uniformized to "size_t", 
> via a gradual but sysmatic cleanup?

Could be.  Can you pick an example and send a patch for it?

Jörn

-- 
Sometimes, asking the right question is already the answer.
-- Unknown

Re: Suggestion on "int len" sanity

[Willy Tarreau]

On Wed, Jun 01, 2005 at 09:06:33AM +0200, XIAO Gang wrote:
> 
> I would like to make a security suggestion.
> 
> There are many length variables in the kernel, locally declared as "len" 
> or "length", either as "int", "unsigned int" or "size_t". However, 
> declaring a length as "int" leads easily to an erroneous situation, as 
> the author (or even a code checker) might make the implicite hypothesis 
> that the length is positive, so that it is enough to make a sanity check 
> of the kind
> 
> if (length > limit) ERROR;
> 
> which is not enough.
> 
> On the other hand, when a variable is named "len" or "length", it is 
> usually used for length and never should go negative. So could I suggest 
> that the declarations of these variables to be uniformized to "size_t", 
> via a gradual but sysmatic cleanup?

Probably true for most cases, but be careful of code which would use
-1 to report some errors if such thing exists.

Willy

Re: Suggestion on "int len" sanity

[Geert Uytterhoeven]

On Wed, 1 Jun 2005, Willy Tarreau wrote:
> On Wed, Jun 01, 2005 at 09:06:33AM +0200, XIAO Gang wrote:
> > I would like to make a security suggestion.
> > 
> > There are many length variables in the kernel, locally declared as "len" 
> > or "length", either as "int", "unsigned int" or "size_t". However, 
> > declaring a length as "int" leads easily to an erroneous situation, as 
> > the author (or even a code checker) might make the implicite hypothesis 
> > that the length is positive, so that it is enough to make a sanity check 
> > of the kind
> > 
> > if (length > limit) ERROR;
> > 
> > which is not enough.
> > 
> > On the other hand, when a variable is named "len" or "length", it is 
> > usually used for length and never should go negative. So could I suggest 
> > that the declarations of these variables to be uniformized to "size_t", 
> > via a gradual but sysmatic cleanup?
> 
> Probably true for most cases, but be careful of code which would use
> -1 to report some errors if such thing exists.

In that case, use ssize_t.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Suggestion on "int len" sanity

[Takashi Iwai]

At Fri, 3 Jun 2005 11:42:23 +0200 (CEST),
Geert Uytterhoeven wrote:
> 
> On Wed, 1 Jun 2005, Willy Tarreau wrote:
> > On Wed, Jun 01, 2005 at 09:06:33AM +0200, XIAO Gang wrote:
> > > I would like to make a security suggestion.
> > > 
> > > There are many length variables in the kernel, locally declared as "len" 
> > > or "length", either as "int", "unsigned int" or "size_t". However, 
> > > declaring a length as "int" leads easily to an erroneous situation, as 
> > > the author (or even a code checker) might make the implicite hypothesis 
> > > that the length is positive, so that it is enough to make a sanity check 
> > > of the kind
> > > 
> > > if (length > limit) ERROR;
> > > 
> > > which is not enough.
> > > 
> > > On the other hand, when a variable is named "len" or "length", it is 
> > > usually used for length and never should go negative. So could I suggest 
> > > that the declarations of these variables to be uniformized to "size_t", 
> > > via a gradual but sysmatic cleanup?
> > 
> > Probably true for most cases, but be careful of code which would use
> > -1 to report some errors if such thing exists.
> 
> In that case, use ssize_t.

In some cases, we may want to avoid [s]size_t because it varies on 32
and 64bit archs (e.g. ioctl parameters)...


Takashi

Re: Suggestion on "int len" sanity

[XIAO Gang]

Willy Tarreau wrote:

>> On the other hand, when a variable is named "len" or "length", it is 
>> usually used for length and never should go negative. So could I suggest 
>> that the declarations of these variables to be uniformized to "size_t", 
>> via a gradual but sysmatic cleanup?

> Probably true for most cases, but be careful of code which would use
> -1 to report some errors if such thing exists.

I agree that they are probably not all replaceable, and care must be taken.

Examples:

1. In the types of sys_[gs]ethostname, sys_[gs]etdomainname, "int len" could be replaced
by "unsigned int" or "size_t" and sanity check simplified.

2. In mm/shmem.c, shmem_symlink(), we have "len = strlen(symname) + 1;". Although it is highly
improbable that strlen(symname) overflows, it is more correct to declare "size_t len;".

3. The similar situation occurs in fs/namei.c, vfs_readlink(). Here it does not matter if len
is declared to be unsigned, but for size_t, we have to take care about the size of size_t.

-- 

XIAO Gang (~{P$8U~})                          xiao@unice.fr
          home page: pcmath126.unice.fr/xiao.html 

Re: Suggestion on "int len" sanity

[Jörn Engel]

On Thu, 2 June 2005 09:28:55 +0200, XIAO Gang wrote:
> 
> Examples:
> 
> 1. In the types of sys_[gs]ethostname, sys_[gs]etdomainname, "int len" 
> could be replaced
> by "unsigned int" or "size_t" and sanity check simplified.

If you really want that fun, try changing it to "unsigned long long"
on your private machine and do some testing.

Hint: arch/i386/kernel/syscall_table.S

> 2. In mm/shmem.c, shmem_symlink(), we have "len = strlen(symname) + 1;". 
> Although it is highly
> improbable that strlen(symname) overflows, it is more correct to declare 
> "size_t len;".

Yep, looks sane.

> 3. The similar situation occurs in fs/namei.c, vfs_readlink(). Here it does 
> not matter if len
> is declared to be unsigned, but for size_t, we have to take care about the 
> size of size_t.

You could possibly change the code to:

int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen, const char *link)
{
	union {
		unsigned len;
		int ret;
	} u;

	u.ret = PTR_ERR(link);
	if (IS_ERR(link))
		goto out;

	u.len = strlen(link);
	if (u.len > (unsigned) buflen)
		u.len = buflen;
	if (copy_to_user(buffer, link, u.len))
		u.ret = -EFAULT;
out:
	return u.ret;
}

But what would we gain, except for a few additional lines?

Jörn

-- 
Happiness isn't having what you want, it's wanting what you have.
-- unknown

Re: Suggestion on "int len" sanity

[XIAO Gang]

Jörn Engel wrote:

>On Thu, 2 June 2005 09:28:55 +0200, XIAO Gang wrote:
>  
>
>>Examples:
>>
>>1. In the types of sys_[gs]ethostname, sys_[gs]etdomainname, "int len" 
>>could be replaced
>>by "unsigned int" or "size_t" and sanity check simplified.
>>    
>>
>
>If you really want that fun, try changing it to "unsigned long long"
>on your private machine and do some testing.
>
>Hint: arch/i386/kernel/syscall_table.S
>
I know; I might try to do something later. The question here is to find 
the best balancing point between what
are better replaced and what are not. I would hesitate a lot before 
doing things as below which are more likely to introduce new bugs than 
to avoid old ones.

>>3. The similar situation occurs in fs/namei.c, vfs_readlink(). Here it does 
>>not matter if len
>>is declared to be unsigned, but for size_t, we have to take care about the 
>>size of size_t.
>>    
>>
>
>You could possibly change the code to:
>
>int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen, const char *link)
>{
>	union {
>		unsigned len;
>		int ret;
>	} u;
>
>	u.ret = PTR_ERR(link);
>	if (IS_ERR(link))
>		goto out;
>
>	u.len = strlen(link);
>	if (u.len > (unsigned) buflen)
>		u.len = buflen;
>	if (copy_to_user(buffer, link, u.len))
>		u.ret = -EFAULT;
>out:
>	return u.ret;
>}
>
>But what would we gain, except for a few additional lines?
>
>Jörn
>
>  
>


-- 

XIAO Gang (~{P$8U~})                          xiao@unice.fr
          home page: pcmath126.unice.fr/xiao.html 

Re: Suggestion on "int len" sanity

[Jörn Engel]

On Thu, 2 June 2005 11:12:13 +0200, XIAO Gang wrote:
> >
> I know; I might try to do something later. The question here is to find 
> the best balancing point between what
> are better replaced and what are not. I would hesitate a lot before 
> doing things as below which are more likely to introduce new bugs than 
> to avoid old ones.

In some cases a comment like "this sucks, but we have to do it
because.." helps.  Example 2 can also be changed to size_t, that one
makes sense.

"grep 'int len'" really is a decent code checker, I agree.  The hard
part is finding the 2% real problems among the output.  And possibly
other cases where s/int/size_t/ doesn't fix anything, but makes
slightly more sense.

Jörn

-- 
Man darf nicht das, was uns unwahrscheinlich und unnatürlich erscheint,
mit dem verwechseln, was absolut unmöglich ist.
-- Carl Friedrich Gauß

Re: Suggestion on "int len" sanity

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1330 bytes --]

On Thu, 2 Jun 2005, [iso-8859-1] Jörn Engel wrote:
> On Thu, 2 June 2005 09:28:55 +0200, XIAO Gang wrote:
> > 3. The similar situation occurs in fs/namei.c, vfs_readlink(). Here it does 
> > not matter if len
> > is declared to be unsigned, but for size_t, we have to take care about the 
> > size of size_t.
> 
> You could possibly change the code to:
> 
> int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen, const char *link)
> {
> 	union {
> 		unsigned len;
                ^^^^^^^^
Plain unsigned is deprecated.

> 		int ret;
> 	} u;

Ugh...

> 
> 	u.ret = PTR_ERR(link);
> 	if (IS_ERR(link))
> 		goto out;
> 
> 	u.len = strlen(link);
> 	if (u.len > (unsigned) buflen)
> 		u.len = buflen;
> 	if (copy_to_user(buffer, link, u.len))
> 		u.ret = -EFAULT;
> out:
> 	return u.ret;
> }

buflen should be size_t.

Since the return value may be negative, it should be signed. But int is not an
option, since size_t is 64 bit on 64-bit machines, while int is still 32-bit.
So the return type should be ssize_t.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Suggestion on "int len" sanity

[Al Viro]

On Fri, Jun 03, 2005 at 11:45:51AM +0200, Geert Uytterhoeven wrote:
> On Thu, 2 Jun 2005, [iso-8859-1] J?rn Engel wrote:
> > int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen, const char *link)
> > {
> > 	union {
> > 		unsigned len;
>                 ^^^^^^^^
> Plain unsigned is deprecated.
> 
> > 		int ret;
> > 	} u;
> 
> Ugh...

Ugh, indeed - who the hell had come up with _that_?

> > 
> > 	u.ret = PTR_ERR(link);
> > 	if (IS_ERR(link))
> > 		goto out;
> > 
> > 	u.len = strlen(link);
> > 	if (u.len > (unsigned) buflen)
> > 		u.len = buflen;
> > 	if (copy_to_user(buffer, link, u.len))
> > 		u.ret = -EFAULT;
> > out:
> > 	return u.ret;
> > }
> 
> buflen should be size_t.
> 
> Since the return value may be negative, it should be signed. But int is not an
> option, since size_t is 64 bit on 64-bit machines, while int is still 32-bit.
> So the return type should be ssize_t.

BS from the beginning to end - we are talking about symlink bodies.
These are *not* going to be 2Gb long.

And that quite neatly shows why the whole plan is a non-starter - you need
to understand what's going on in the code being mangled.

Re: Suggestion on "int len" sanity

[Jörn Engel]

On Fri, 3 June 2005 13:20:58 +0100, Al Viro wrote:
> 
> Ugh, indeed - who the hell had come up with _that_?

/me did.

Should I add explicit <joke> tags next time?  Thought it would be
obvious, esp. to you.

Jörn

-- 
I don't understand it. Nobody does.
-- Richard P. Feynman

Re: Suggestion on "int len" sanity

[Andreas Schwab]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

>> 	union {
>> 		unsigned len;
>                 ^^^^^^^^
> Plain unsigned is deprecated.

Says who?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Suggestion on "int len" sanity

[Geert Uytterhoeven]

On Fri, 3 Jun 2005, Andreas Schwab wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> 
> >> 	union {
> >> 		unsigned len;
> >                 ^^^^^^^^
> > Plain unsigned is deprecated.
> 
> Says who?

Sorry, forgot to add the
`Signed-Off-by: Geert Uytterhoeven <geert@linux-m68k.org>' line :-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: Suggestion on "int len" sanity

[Andreas Schwab]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Fri, 3 Jun 2005, Andreas Schwab wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> 
>> >> 	union {
>> >> 		unsigned len;
>> >                 ^^^^^^^^
>> > Plain unsigned is deprecated.
>> 
>> Says who?
>
> Sorry, forgot to add the
> `Signed-Off-by: Geert Uytterhoeven <geert@linux-m68k.org>' line :-)

Who deprecated it?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Suggestion on "int len" sanity

[randy_dunlap]

On Fri, 03 Jun 2005 16:38:07 +0200 Andreas Schwab wrote:

| Geert Uytterhoeven <geert@linux-m68k.org> writes:
| 
| > On Fri, 3 Jun 2005, Andreas Schwab wrote:
| >> Geert Uytterhoeven <geert@linux-m68k.org> writes:
| >> 
| >> >> 	union {
| >> >> 		unsigned len;
| >> >                 ^^^^^^^^
| >> > Plain unsigned is deprecated.
| >> 
| >> Says who?
| >
| > Sorry, forgot to add the
| > `Signed-Off-by: Geert Uytterhoeven <geert@linux-m68k.org>' line :-)
| 
| Who deprecated it?

Not technically deprecated, just undesirable in the kernel sources.


---
~Randy

Re: Suggestion on "int len" sanity

[Lars Marowsky-Bree]

On 2005-06-03T11:04:36, randy_dunlap <rdunlap@xenotime.net> wrote:

> | > Sorry, forgot to add the
> | > `Signed-Off-by: Geert Uytterhoeven <geert@linux-m68k.org>' line :-)
> | 
> | Who deprecated it?
> 
> Not technically deprecated, just undesirable in the kernel sources.

Don't tell the device mapper folks. Uhm. And why is it depreciated?


-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business	 -- Charles Darwin
"Ignorance more frequently begets confidence than does knowledge"

Re: Suggestion on "int len" sanity

[randy_dunlap]

On Fri, 3 Jun 2005 20:09:41 +0200 Lars Marowsky-Bree wrote:

| On 2005-06-03T11:04:36, randy_dunlap <rdunlap@xenotime.net> wrote:
| 
| > | > Sorry, forgot to add the
| > | > `Signed-Off-by: Geert Uytterhoeven <geert@linux-m68k.org>' line :-)
| > | 
| > | Who deprecated it?
| > 
| > Not technically deprecated, just undesirable in the kernel sources.
| 
| Don't tell the device mapper folks. Uhm. And why is it depreciated?

what's depreciated?

Kernel source prefers more explicit typing/sizing,
that's all that I can offer.

And the the C language lawyers can say that 'unsigned' is explicit.  :(

---
~Randy

Re: Suggestion on "int len" sanity

[Christoph Hellwig]

On Fri, Jun 03, 2005 at 11:04:36AM -0700, randy_dunlap wrote:
> | Who deprecated it?
> 
> Not technically deprecated, just undesirable in the kernel sources.

Who claims that?

Re: Suggestion on "int len" sanity

[randy_dunlap]

On Sat, 4 Jun 2005 12:28:25 +0100 Christoph Hellwig wrote:

| On Fri, Jun 03, 2005 at 11:04:36AM -0700, randy_dunlap wrote:
| > | Who deprecated it?
| > 
| > Not technically deprecated, just undesirable in the kernel sources.
| 
| Who claims that?

I dunno, but I'm not religious about it.

It's certainly not in CodingStyle, but I'm sure that I'm not just
dreaming it (not making it up), I've seen it in emails from some
maintainers, but searching for /unsigned/ generates too many hits
to follow.

---
~Randy

Re: Suggestion on "int len" sanity

[Jörn Engel]

On Sat, 4 June 2005 09:58:11 -0700, randy_dunlap wrote:
> On Sat, 4 Jun 2005 12:28:25 +0100 Christoph Hellwig wrote:
> | 
> | Who claims that?
> 
> I dunno, but I'm not religious about it.
> 
> It's certainly not in CodingStyle, but I'm sure that I'm not just
> dreaming it (not making it up), I've seen it in emails from some
> maintainers, but searching for /unsigned/ generates too many hits
> to follow.

Which means, we can safely ignore this issue.  If "some maintainers"
really care, this thing will resurface.

Jörn

-- 
Homo Sapiens is a goal, not a description.
-- unknown

Re: Suggestion on "int len" sanity

[randy_dunlap]

On Sat, 4 Jun 2005 19:07:58 +0200 Jörn Engel wrote:

| On Sat, 4 June 2005 09:58:11 -0700, randy_dunlap wrote:
| > On Sat, 4 Jun 2005 12:28:25 +0100 Christoph Hellwig wrote:
| > | 
| > | Who claims that?
| > 
| > I dunno, but I'm not religious about it.
| > 
| > It's certainly not in CodingStyle, but I'm sure that I'm not just
| > dreaming it (not making it up), I've seen it in emails from some
| > maintainers, but searching for /unsigned/ generates too many hits
| > to follow.
| 
| Which means, we can safely ignore this issue.  If "some maintainers"
| really care, this thing will resurface.

Certainly.

---
~Randy