Unsafe use of fixed-size buffer

[devkev]

The static buffer result_buffer is declared to have size 256, but the unsafe functions sprintf() and strcpy() are used instead of snprintf() and strncpy(). For long results (such as on my Dell L702X when powering on the discrete graphics), this causes result_buffer to be overflowed, stomping on whatever kernel memory lies after it. On kernels after 2.6.38, this causes a BUG for me, but the fact that it works at all on earlier kernels is a fluke (and simply adding some debugging printks can make 2.6.38+ "work").

temporary_buffer might also be affected, but I haven't checked it.

At the very least, increase the 256 to something larger like 2048. Preferably, fix the code to handle this array correctly and avoid any overflows. (I might attempt a patch if I get time, for now I have simply increased the size so as to get things working for the time being on my system.)

[ArchangeGabriel]

Could you write this patch please ?

I'm not having a good knwoledge of C, but there is effectively something to change here.

[mkottman]

I am sorry, but I do not have the time to take care of acpi_call right now. It all started as a proof-of-concept that "went into production" too soon.

I am happy to accept any patches to acpi_call to make it safer/more efficienty/easier to use or otherwise better. Or the github way - create a fork, commit the code and issue a pull request - I will be happy to accept it.

[Lekensteyn]

I'm working on a patch that verifies the sizes of result_buffer and temporary_buffer. I'll trim the buffer data if the buffer is too small and add , .... instead of , 0xXX when the buffer is trimmed.

[Lekensteyn]

Nevermind, I'm going to replace the last } character with a comma if the buffer is too small. This saves another bit of data.

[devkev]

My kernel-fu isn't strong enough to do this, but maybe you could just dynamically allocate a buffer of the right size? The size is known: for strings it's result->string.length, and for buffers it's result->buffer.length (where you'd need 6*n+1, since 6 chars per u8). Using snprintf and strncpy, etc would still help, though.

[Lekensteyn]

So far I've got https://github.com/Bumblebee-Project/acpi_call/tree/fix-buffer-overflow I think acpi_proc_read needs to be rewritten in case result_buffer is bigger than the size of the page char pointer. To be continued tomorrow.

[Lekensteyn]

I stand corrected, acpi_proc_read is fine. As long as BUFFER_SIZE is lower than the size of a page, it's OK (page size is 4096 bytes on x86 and varies between 4, 8, 16 and 64KiB on ia64.

acpi_proc_write has one thing that confuses me: why is the input buffer set to a size of 512 if the buffers are set to BUFFER_SIZE (=256)? If it's just to accept the trailing newline, it should be set to BUFFER_SIZE because the newline does not have to be added (i.e. using printf "%s" 'method here).

temporary_buffer is already protected against buffer overflow, but floods the kernel log if the buffer is too small. I've confirmed this in VirtualBox. For experimental purposes, I set BUFFER_SIZE to 32 and ran the following:

# printf '\_SB.PCI0.HDEF._DSM 0 0 0 {0 0 0 0 0  0 0 0 0 0  0 0 0 0 0  0 0 0 0 0  0 0 0 0 0  0 0 0 0 0  0 0 0 0;' > /proc/acpi/call ;dmesg | tail -4
acpi_call: buffer arg4 is truncated because the buffer is full
acpi_call: buffer arg4 is truncated because the buffer is full
acpi_call: Calling \_SB.PCI0.HDEF._DSM
acpi_call: Call successful: {0x00}

I'll fix the duplicate messages in the next commit.

For future reference, the documentation of read_proc in the kernel source.

[Lekensteyn]

@mkottman please review and merge: https://github.com/Bumblebee-Project/acpi_call/compare/master...fix-buffer-overflow

@devkev: could you test the patch?

[devkev]

Confirmed working on my laptop in 3.0.3, thanks very much.

[Lekensteyn]

Pull request fixing this issue at #20