Sound updating, security of strlcpy and a question on pci v unload

Sound updating, security of strlcpy and a question on pci v unload

[Alan Cox]

I'm currently updating the prehistoric OSS audio code in 2.5 to include
all the new 2.4 drivers and 2.4 work. While some of them overlap ALSA
drivers others are not in ALSA yet either.

Firstly someone turned half the kernel into using strlcpy. Every single
change I looked at bar two in the sound layer introduced a security
hole. It looks like whoever did it just fired up a perl macro without
realising the strncpy properties matter for data copied to user space.
Looks like the rest wants auditing

Secondly a question. pci_driver structures seem to lack an owner: field.
What stops a 2.5 module unload occuring while pci is calling the probe
function having seen a new device ? 


-- 
Alan Cox <alan@lxorguk.ukuu.org.uk>

Re: Sound updating, security of strlcpy and a question on pci v unload

[Mikulas Patocka]

> I'm currently updating the prehistoric OSS audio code in 2.5 to include
> all the new 2.4 drivers and 2.4 work. While some of them overlap ALSA
> drivers others are not in ALSA yet either.
>
> Firstly someone turned half the kernel into using strlcpy. Every single
> change I looked at bar two in the sound layer introduced a security
> hole. It looks like whoever did it just fired up a perl macro without
> realising the strncpy properties matter for data copied to user space.
> Looks like the rest wants auditing

What's the difference there? strlcpy always creates null-terminated
string, strncpy doesn't. strncpy in kernel (unlike user strncpy) does not
pad the whole destination buffer with zeros (see comment and
implementation in lib/string.c), so I don't see any point why strncpy
should be more secure.

Mikulas

SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

On Gwe, 2003-07-11 at 20:04, Mikulas Patocka wrote:
> What's the difference there? strlcpy always creates null-terminated
> string, strncpy doesn't. strncpy in kernel (unlike user strncpy) does not
> pad the whole destination buffer with zeros (see comment and
> implementation in lib/string.c), so I don't see any point why strncpy
> should be more secure.

Lots of kernel drivers rely on the libc definition of strncpy. 

Lets update the bug report to "2.4 and 2.5 both leak arbitary kernel data
to user space" tho thankfully in small pieces. Fix required. (bcc'd to Mark to 
assign a CAN number)

And for 2.4-ac I'm going to simply go make strncpy do what it says in the
book. For 2.5 the same is true and cleaner (since those who use strlcpy
properly don't take any performance hit). Actually it may make sense to 
backport strlcpy for those odd performance critical ones.

I don't think its that serious a bug - the odds of getting a critical bit of
someone elses data are remarkably low but it wants fixing.

Alan

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

Ok an update:

2.4 you have a problem if your port uses the lib/string.c
implementation. x86 does not and is ok (very nifty implementation of the
zeroing too)

That appears to be: 

Not vulnerable: x86 
Buggy generic: ARM, CRIS, IA-64, PA-RISC, S/390, SH64, SPARC, SPARC64,
X86-64
Buggy asm: m68k, mips (?), sh, 
Unknown: alpha, ppc

2.5 you have problems all over the place from wrong strlcpy conversions,
but those are easy enough to clean up before 2.6.0

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Paul Mackerras]

Alan Cox writes:

> Unknown: alpha, ppc

PPC doesn't clear the rest of the destination, I'll fix it.

Paul.

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Horst von Brand]

Alan Cox <alan@lxorguk.ukuu.org.uk> said:

[...]

> 2.5 you have problems all over the place from wrong strlcpy conversions,
> but those are easy enough to clean up before 2.6.0

Perhaps there should be a strncpy_touser() to make it crystal clear that it
_can't_ be "optimized" into strlcpy()
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

On Sad, 2003-07-12 at 22:28, Horst von Brand wrote:
> Perhaps there should be a strncpy_touser() to make it crystal clear that it
> _can't_ be "optimized" into strlcpy()

The direct to_user functions are not normally a problem. The data they fail
to clear is the users own data anyway.

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Linus Torvalds]

On 11 Jul 2003, Alan Cox wrote:
> 
> Lots of kernel drivers rely on the libc definition of strncpy. 

But that's ok. We _do_ do the padding. I hated it when I wrote it, but as 
far as I know, the kernel strncpy() has done padding pretty much since day 
one.

Yes, strlcpy() conversion users need to be careful, but I think we mostly 
_were_ careful. Knock wood.

		Linus

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

On Gwe, 2003-07-11 at 23:44, Linus Torvalds wrote:
> On 11 Jul 2003, Alan Cox wrote:
> > 
> > Lots of kernel drivers rely on the libc definition of strncpy. 
> 
> But that's ok. We _do_ do the padding. I hated it when I wrote it, but as 
> far as I know, the kernel strncpy() has done padding pretty much since day 
> one.

/**
 * strncpy - Copy a length-limited, %NUL-terminated string
 * @dest: Where to copy the string to
 * @src: Where to copy the string from
 * @count: The maximum number of bytes to copy
 *
 * Note that unlike userspace strncpy, this does not %NUL-pad the buffer.
 * However, the result is not %NUL-terminated if the source exceeds
 * @count bytes.
 */

Only x86 does the padding 

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Pete Zaitcev]

On 11 Jul 2003 23:50:15 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Gwe, 2003-07-11 at 23:44, Linus Torvalds wrote:
> > On 11 Jul 2003, Alan Cox wrote:
> > > 
> > > Lots of kernel drivers rely on the libc definition of strncpy. 
> > 
> > But that's ok. We _do_ do the padding. I hated it when I wrote it, but as 
> > far as I know, the kernel strncpy() has done padding pretty much since day 
> > one.

>  * Note that unlike userspace strncpy, this does not %NUL-pad the buffer.
>  * However, the result is not %NUL-terminated if the source exceeds
>  * @count bytes.
>  */
> 
> Only x86 does the padding 

I do not undestand Alan's position, if he is for it or against it.
Anyway, in case you want it, here's what I wrote for s390.
I wrote some userland tests, it seems to check out. BUT I warn you,
someone better check my assembly.

-- Pete

diff -ur -X dontdiff linux-2.6.1/arch/s390/lib/strncpy64.S linux-2.6.1-s390/arch/s390/lib/strncpy64.S
--- linux-2.6.1/arch/s390/lib/strncpy64.S	2003-07-13 20:31:50.000000000 -0700
+++ linux-2.6.1-s390/arch/s390/lib/strncpy64.S	2004-01-28 18:48:27.000000000 -0800
@@ -23,8 +23,16 @@
 	LA      3,1(3)
         STC     0,0(1)
 	LA      1,1(1)
-        JZ      strncpy_exit   # ICM inserted a 0x00
+        JZ      strncpy_pad    # ICM inserted a 0x00
         BRCTG   4,strncpy_loop # R4 -= 1, jump to strncpy_loop if > 0
-strncpy_exit:
         BR      14
 
+strncpy_pad:
+	LTR     4,4
+        JZ      strncpy_exit   # 0 bytes -> nothing to do
+strncpy_padloop:
+	MVI	0(1),0
+	LA	1,1(1)
+	BRCTG	4,strncpy_padloop
+strncpy_exit:
+        BR      14
diff -ur -X dontdiff linux-2.6.1/arch/s390/lib/strncpy.S linux-2.6.1-s390/arch/s390/lib/strncpy.S
--- linux-2.6.1/arch/s390/lib/strncpy.S	2003-07-13 20:35:16.000000000 -0700
+++ linux-2.6.1-s390/arch/s390/lib/strncpy.S	2004-01-28 18:46:20.000000000 -0800
@@ -23,8 +23,16 @@
 	LA      3,1(3)
         STC     0,0(1)
 	LA      1,1(1)
-        JZ      strncpy_exit   # ICM inserted a 0x00
+        JZ      strncpy_pad    # ICM inserted a 0x00
         BRCT    4,strncpy_loop # R4 -= 1, jump to strncpy_loop if >  0
-strncpy_exit:
         BR      14
 
+strncpy_pad:
+	LTR	4,4
+	JZ	strncpy_exit
+strncpy_padloop:
+	MVI	0(1),0
+	LA	1,1(1)
+	BRCT	4,strncpy_padloop
+strncpy_exit:
+        BR      14

Re: Sound updating, security of strlcpy and a question on pci v unload

[Mitchell Blank Jr]

Mikulas Patocka wrote:
> What's the difference there? strlcpy always creates null-terminated
> string, strncpy doesn't. strncpy in kernel (unlike user strncpy) does not
> pad the whole destination buffer with zeros (see comment and
> implementation in lib/string.c), so I don't see any point why strncpy
> should be more secure.

Not only that, I think the point is usually moot anyway.  If you're
filling in a structure to pass to userspace like:

	struct whatever foo;
	strncpy(foo.name, "My Driver", sizeof(foo.name));
	foo.count = 1;
	[...]

then you're STILL probably at risk of data leakage if "struct whatever"
requires padding on any architecture.  The real fix is to make sure
that "foo" is explicitly zero'ed out first.  Then strlcpy-vs-strncpy
becomes a non-issue.

I wonder if strncpy() should just be removed from the kernel since it
doesn't seem to behave consistently across architectures anyway.  There's
probably only a couple places that actually ever would WANT to generate
a maybe-NUL-terminated byte array and they could just open code it.
For 95%+ of cases strlcpy() is the better API.

-Mitch

Re: Sound updating, security of strlcpy and a question on pci v unload

[Greg KH]

On Fri, Jul 11, 2003 at 06:05:37PM +0100, Alan Cox wrote:
> 
> Secondly a question. pci_driver structures seem to lack an owner: field.
> What stops a 2.5 module unload occuring while pci is calling the probe
> function having seen a new device ? 

The pci subsystem rw semaphore.

thanks,

greg k-h

Re: Sound updating, security of strlcpy and a question on pci v unload

[Albert Cahalan]

Mitchell Blank Jr writes:
> Mikulas Patocka wrote:

>> What's the difference there? strlcpy always creates null-terminated
>> string, strncpy doesn't. strncpy in kernel (unlike user strncpy) does not
>> pad the whole destination buffer with zeros (see comment and
>> implementation in lib/string.c), so I don't see any point why strncpy
>> should be more secure.
>
> Not only that, I think the point is usually moot anyway.
> If you're filling in a structure to pass to userspace like:
>
>  struct whatever foo;
>  strncpy(foo.name, "My Driver", sizeof(foo.name));
>  foo.count = 1;
>  [...]
>
> then you're STILL probably at risk of data leakage if "struct whatever"
> requires padding on any architecture.  The real fix is to make sure
> that "foo" is explicitly zero'ed out first.  Then strlcpy-vs-strncpy
> becomes a non-issue.

gcc -Wpadding ...

If the compiler has to add padding, then the programmer
most likely messed up. The warning catches stupidity.
Necessary padding can be explicit.