Unsafe use of fixed-size buffer about acpi_call

[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:
Bumblebee-Project/acpi_call@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