[prev in list] [next in list] [prev in thread] [next in thread]
List: fwts-devel
Subject: Re: ACK: [PATCH][v3] efi_runtime: ensure we don't allocate a zero byte buffer (LP: #1429890)]
From: Keng-Yu Lin <keng-yu.lin () canonical ! com>
Date: 2015-03-18 4:43:02
Message-ID: CADXHx7b_UxDUst92bNcVMZLvUsfkFjwiwwta1FeNyQM0ZWT5XQ () mail ! gmail ! com
[Download RAW message or body]
On Wed, Mar 18, 2015 at 11:54 AM, Alex Hung <alex.hung@canonical.com> wrote:
> On 03/16/2015 09:27 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> When requesting a test on a zero sized buffer we must ensure that a
>> buffer is allocated and is of zero size when passed to the EFI service.
>>
>> If we kalloc zero bytes, then a ZERO_SIZE_PTR is returned, which has
>> no meaning to the EFI service, so one strategy is to convert this to
>> a NULL before passing it to the EFI service. However, this is handled
>> by the service as an invalid parameter and returns with
>> EFI_INVALID_PARAMETER rather than EFI_BUFFER_TOO_SMALL (the latter
>> being what we are trying to actually exercise).
>>
>> The best way forward is to allocate a buffer that is 1 wide char too
>> long, and return back the buffer offset by 1 wide char (i.e skipping
>> the leading 2 bytes). This allows us to always return a valid sized
>> buffer that is passed to the EFI service, even with zero length requests.
>> For zero sized buffers, if EFI writes into the zero sized buffer it will
>> corrupt the member following the buffer. I believe further checks should
>> probably be added to also sanity check for these potential buffer overruns.
>>
>> Across various platforms this arrangement trips the expected
>> EFI_BUFFER_TOO_SMALL error return, as originally expected in the fwts userspace
>> test case.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> efi_runtime/efi_runtime.c | 44 +++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
>> index ff31685..2708b4b 100644
>> --- a/efi_runtime/efi_runtime.c
>> +++ b/efi_runtime/efi_runtime.c
>> @@ -133,6 +133,15 @@ static inline size_t __ucs2_strsize(uint16_t __user *str)
>> }
>>
>> /*
>> + * Free a buffer allocated by copy_ucs2_from_user_len()
>> + */
>> +static inline void ucs2_kfree(uint16_t *buf)
>> +{
>> + if (buf)
>> + kfree(buf - 1);
>> +}
>> +
>> +/*
>> * Allocate a buffer and copy a ucs2 string from user space into it.
>> *
>> * We take an explicit number of bytes to use for the allocation and
>> @@ -141,11 +150,22 @@ static inline size_t __ucs2_strsize(uint16_t __user *str)
>> *
>> * If a non-zero value is returned, the caller MUST NOT access 'dst'.
>> *
>> - * It is the caller's responsibility to free 'dst'.
>> + * It is the caller's responsibility to free 'dst' using ucs2_kfree()
>> + *
>> + * We cater for zero sized len by always allocating a buffer that is 1
>> + * uint16_t larger than the requested size and passing back the buffer
>> + * offset by 1 uint16_t. That way, the returned buffer size is the
>> + * size that is requested and the buffer ptr is a valid pointer (and not
>> + * NULL or ZERO_SIZE_PTR). This allows EFI services to be passed a valid
>> + * allocated buffer of zero length size and to see if the services handle
>> + * this as an EFI_BUFFER_TOO_SMALL error.
>> + *
>> */
>> static inline int
>> copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len)
>> {
>> + uint16_t *buf;
>> +
>> if (!src) {
>> *dst = NULL;
>> return 0;
>> @@ -154,12 +174,15 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len)
>> if (!access_ok(VERIFY_READ, src, 1))
>> return -EFAULT;
>>
>> - *dst = kmalloc(len, GFP_KERNEL);
>> - if (!*dst)
>> + buf = kmalloc(len + sizeof(uint16_t), GFP_KERNEL);
>> + if (!buf) {
>> + *dst = 0;
>> return -ENOMEM;
>> + }
>> + *dst = buf + 1;
>>
>> if (copy_from_user(*dst, src, len)) {
>> - kfree(*dst);
>> + kfree(buf);
>> return -EFAULT;
>> }
>>
>> @@ -242,14 +265,13 @@ static long efi_runtime_get_variable(unsigned long arg)
>>
>> data = kmalloc(datasize, GFP_KERNEL);
>> if (!data) {
>> - kfree(name);
>> + ucs2_kfree(name);
>> return -ENOMEM;
>> }
>>
>> prev_datasize = datasize;
>> status = efi.get_variable(name, &vendor, &attr, &datasize, data);
>> -
>> - kfree(name);
>> + ucs2_kfree(name);
>>
>> if (status == EFI_SUCCESS && prev_datasize >= datasize)
>> rv = copy_to_user(pgetvariable_local.Data, data, datasize);
>> @@ -301,12 +323,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>>
>> data = kmalloc(psetvariable_local.DataSize, GFP_KERNEL);
>> if (!data) {
>> - kfree(name);
>> + ucs2_kfree(name);
>> return -ENOMEM;
>> }
>> if (copy_from_user(data, psetvariable_local.Data,
>> psetvariable_local.DataSize)) {
>> - kfree(data);
>> + ucs2_kfree(data);
>> kfree(name);
>> return -EFAULT;
>> }
>> @@ -315,7 +337,7 @@ static long efi_runtime_set_variable(unsigned long arg)
>> psetvariable_local.DataSize, data);
>>
>> kfree(data);
>> - kfree(name);
>> + ucs2_kfree(name);
>>
>> if (put_user(status, psetvariable_local.status))
>> return -EFAULT;
>> @@ -469,7 +491,7 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
>> if (status == EFI_SUCCESS && prev_name_size >= name_size)
>> rv = copy_ucs2_to_user_len(pgetnextvariablename_local.VariableName,
>> name, name_size);
>> - kfree(name);
>> + ucs2_kfree(name);
>>
>> if (rv)
>> return -EFAULT;
>>
>
>
> Acked-by: Alex Hung <alex.hung@canonical.com>
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>
--
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic