Hi,
On 1/17/23 18:06, Hans de Goede wrote:
> 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) {
Correction should be:
if (out == dst_size) {
> pr_warn("UTF16 string too long, truncated\n");
> out--;
> }
>
> dst[out] = 0;
>
> *buffer += src_size * sizeof(u16);
> *buffer_size -= in * sizeof(u16);
Correction this line should be:
*buffer_size -= src_size * sizeof(u16);
Normally these will be the same but if we run out of output
space then that might break the loop and then in < src_size.
> 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 ...
>
>
