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 <[email protected]>
> 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
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
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
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
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
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
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
Alan Cox writes:
> Unknown: alpha, ppc
PPC doesn't clear the rest of the destination, I'll fix it.
Paul.
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.
Alan Cox <[email protected]> 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
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.
On 11 Jul 2003 23:50:15 +0100
Alan Cox <[email protected]> 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
Hi Pete,
> 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.
Learning to write inline assembly? Nice, but it has one small
problem, the count in %r4 is not decremented for 0x00 byte.
Try my little patch.
blue skies,
Martin.
diff -urN 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 Fri Jan 9 07:59:45 2004
+++ linux-2.6.1-s390/arch/s390/lib/strncpy.S Thu Jan 29 09:53:02 2004
@@ -23,8 +23,13 @@
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_clear:
+ STC 0,0(1)
+ LA 1,1(1)
+strncpy_pad:
+ BRCT 4,strncpy_clear
+ BR 14
diff -urN 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 Fri Jan 9 07:59:10 2004
+++ linux-2.6.1-s390/arch/s390/lib/strncpy64.S Thu Jan 29 09:53:02 2004
@@ -23,8 +23,13 @@
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_clear:
+ STC 0,0(1)
+ LA 1,1(1)
+strncpy_pad:
+ BRCTG 4,strncpy_clear
+ BR 14