On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:

quoted

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
Sent: Friday, March 16, 2012 1:46 AM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
Alan Stern
Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
messages in the driver

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.

The problem is that off-by-ones tend to reproduce by copy and paste.
It's best to never introduce any, even harmless ones.

Either that or add a comment.  /* Don't care about wrong limitter
because we trust the input. */.

With regards to the negative values, negative values indicate a failure of some sort
in the conversion. Since the host is the recipient here, host will correctly deal with the
transaction by discarding the tuple.  

I'm not super familiar with this subsystem.  Where can I find code
for rejecting bad transactions?  It seems like an easy thing to
handle the error in both places.  It makes auditing the code a lot
simpler.

regards,
dan carpenter