On Fri, Mar 16, 2012 at 10:18:58AM +0300, Dan Carpenter wrote:

quoted

quoted

On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:

quoted

 	/*
 	 * The windows host expects the key/value pair to be encoded
 	 * in utf16.
 	 */
 	keylen = utf8s_to_utf16s(key_name, strlen(key_name),

UTF16_HOST_ENDIAN,

quoted

-				(wchar_t *) kvp_data->data.key,
+				(wchar_t *) kvp_data->key,
 				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
-	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
+	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
+

I feel like a jerk for asking this, but is the output length correct
here?  It seems like we could go over again.  Also utf8s_to_utf16s()
can return negative error codes, why do we ignore those?

We are returning the strings back to the host here. There are checks elsewhere
in the code to ensure that all strings we return to the host can be accommodated
in the available space. For the most part these are strings that the host gave us in the 
first place that have already been validated.  Furthermore, there are checks on the 
host side to ensure that the returned size parameters are consistent with the protocol 
definitions for the key value pair. For instance let us say somehow we got into a 
situation where the converted utf16 string occupied the entire MAX sized array 
without any room for the terminating character and we set the length parameter 
to 2 more than the MAX value as this code would do. The host would simply discard the 
message as an illegal message. This would be more appropriate than sending a 
truncated key or value.

Uh...  Looking at it again, this code is clearly off by one.  If
we're not going to hit the limit, then we're not going to truncate,
so that's not a concern.  Let's just use the correct limit here.

Another option of course would be to add a test after the
conversion.

	if (keylen == HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2)
		return -EINVAL;

What I'm saying is that I audit a lot of code for buffer overflows,
and I don't want to see an off by one and then I have to audit where
the string come from and audit where it's going.

If it's corrupts memory then I fix the bug and I can list it under
my achievements in my weekly status report.  If it's wrong but it
doesn't corrupt memory, it's just a complete waste of my time and it
makes me really cross.

regards,
dan carpenter