Hi Jorge,
On 1/16/23 17:07, Hans de Goede wrote:
> Hi Jorge,
>
> First of all some generic remarks which apply to all patches:
>
> 1. Quite a few non local / non static functions have generic names like e.g.
> calculate_string_buffer().
>
> If this code is build into the main kernel vmlinuz then these will
> all be part of a global namespace, leading to potential symbol
> collisions to avoid these please make the following 2 changes
>
> i) Make functions which are only used in a single .c file static
> e.g. wmi_error_and_message()
> ii) Prefix others with hp_bioscfg_, e.g. hp_bioscfg_calculate_string_buffer()
> to avoid possible symbol clashes
One more generic remark. All the struct string_data, struct integer_data, etc.
structs have an attribute_name member. But except for some leftover code
filling (but never reading) that in spmobj-attributes.c there is no code using
that. Please remove the attribute_name member from all the string_ / integer_ /
enumeration_data, etc. structs.
Also some more issues in this patch which I noticed while looking at patch 3/5:
> +int get_integer_from_buffer(int **buffer, int *buffer_size, int *integer)
> +{
> + int *ptr = PTR_ALIGN(*buffer, 4);
This may move the ptr, but if it does, then buffer_size should be
modified accordingly since you have just consumed some buffer-space
by moving the pointer.
So you will want to add something like this:
int align_size = (char *)ptr - (char *)(*buffer);
> +
> + /* Ensure there is enough space remaining to read the integer */
> + if (*buffer_size < sizeof(int))
> + return -EINVAL;
This check is not correct, because you are not taking the alignment
adjustment which I mentioned above into account instead this should be:
if (*buffer_size < (sizeof(int) + align_size))
return -EINVAL;
> +
> + *integer = *(ptr++);
> + *buffer = ptr;
And this:
> + *buffer_size -= sizeof(int);
Should be:
*buffer_size -= sizeof(int) + align_size;
> +
> + return 0;
> +}
> +
> +int get_string_from_buffer(u16 **buffer, int *buffer_size, char **str)
> +{
> + u16 *ptr = *buffer;
> + u16 ptrlen;
> +
> + u16 size;
> + int i;
> + char *output = NULL;
> + int escape = 0;
> +
What if *buffer_size is 0 at this point already, now you have
just read past the end of the buffer.
> + ptrlen = *(ptr++);
> + size = ptrlen / 2;
> +
> + /* Ensure there is enough space remaining to read and convert
> + * the string
> + */
> + if (*buffer_size < (size * sizeof(u16)))
Use " < ptrlen" here ?
> + return -EINVAL;
> +
> +
> + for (i = 0; i < size; i++)
> + if (ptr[i] == '\\' || ptr[i] == '\r' || ptr[i] == '\n' || ptr[i] == '\t')
> + escape++;
> +
There are a number of issues with the code below.
> + size += escape;
This not only increases the allocated size, but also
causes the for (i = 0; i < size; i++) to loop over the
extra space allocated for escaping special chars, while
it also does:
if (*ptr == '\\' || *ptr == '\r' || *ptr == '\n' || *ptr == '\t')
output[i++] = '\\';
Which increases i, so in the end this will end up writing
size + esc times to output even though the size of output is
only size + 1.
so I think you simply want to put the + escape inside the
kcalloc like this:
*str = kcalloc(size + escape + 1, sizeof(char), GFP_KERNEL);
And keep size at it was (instead of incrementing it with esc).
> + *str = kcalloc(size + 1, sizeof(char), GFP_KERNEL);
> + if (!*str)
> + return -ENOMEM;
> +
> + output = *str;
> +
> + /*
> + * convert from UTF-16 unicode to ASCII
> + */
> + utf16s_to_utf8s(ptr, ptrlen, UTF16_HOST_ENDIAN, output, size);
There are 4 issues with just this single line:
1. The inlen parameter of utf16s_to_utf8s is in wchar_t-s not in bytes,
so you should pass size here, not ptrlen
2. The size of the destination buffer is simply the number of u16-s in
the source string, but a single u16 may expand to a multi-byte UTF8 sequence
so that might be too small.
3. In the for loop below you are completely overwriting the result of this
conversion, so you might just as well not have made this call at all
4. You are not using the return value which may be different from size
because of the multi-byte sequence expansion.
> +
> + for (i = 0; i < size; i++) {
> + if (*ptr == '\\' || *ptr == '\r' || *ptr == '\n' || *ptr == '\t')
> + output[i++] = '\\';
> +
> + if (*ptr == '\r')
> + output[i] = 'r';
> + else if (*ptr == '\n')
> + output[i] = 'n';
> + else if (*ptr == '\t')
> + output[i] = 't';
> + else if (*ptr == '"')
> + output[i] = '\'';
> + else
> + output[i] = *ptr;
> + ptr++;
> + }
You are not properly 0 terminating the string here, since you have
kcalloc-ed the memory this will be fine (if we forget about the possible
buffer overruns when escaping). But it would be better to use a
regular malloc, avoiding needlessly clearing the mem and then explicitly
add the 0 termination here.
> +
> + *buffer = ptr;
> + *buffer_size -= size * sizeof(u16);
This is not taking into account the 2 bytes used by the size header,
those should also be subtracted.
> + return size;
> +}
As for determining outsize, I notice that in all the call sites
of get_string_from_buffer() you are copying the returned string
to a fixed-size destination buffer, so instead of allocating it here,
you can just pass in that fixed size buffer right away, also
removig a needless malloc + strcpy + free in the process.
So all in all I think this function should look something like this
(untested):
int get_string_from_buffer(u8 **buffer, int *buffer_size, char *dst, int dst_size)
{
u16 *src = (u16 *)(*buffer);
int in, out, src_size, len;
if (buffer_size < sizeof(u16))
return -EINVAL;
src_size = *src++;
*buffer_size -= sizeof(u16)
if (*buffer_size < src_size)
return -EINVAL;
/* convert size bytes -> u16 units */
src_size /= sizeof(u16);
in = 0;
out = 0;
while (in < src_size && out < dst_size) {
len = 0;
while (in + len < src_size && src[in + len] != '\\' && src[in + len] != '\r' && src[in + len] != '\n' && src[in + len] != '\t' && src[in + len] != '"')
len++;
if (len) {
out += utf16s_to_utf8s(src + in, len, UTF16_HOST_ENDIAN, dst + out, dst_size - out);
in += len;
continue;
}
/* Special case convert " -> ' */
if (src[in] == '"') {
dst[out++] = '\'';
in++;
continue;
}
dst[out++] = '\\';
if (out == dst_size)
break;
if (src[in] == '\\')
dst[out++] = '\\';
else if (src[in] == '\r')
dst[out++] = 'r';
else if (src[in] == '\n')
dst[out++] = 'n';
else if (src[in] == '\t')
dst[out++] = 't';
in++;
}
if (out == output_size) {
pr_warn("UTF16 string too long, truncated\n");
out--;
}
dst[out] = 0;
*buffer += src_size * sizeof(u16);
*buffer_size -= in * sizeof(u16);
return 0;
}
Note I have also changed the buffer function parameter type from u16 ** to u8**
so that callers don't need to cast it.
Regards,
Hans
I'll continue reviewing this patch-set later, not sure yet when ...
