On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote: > > > > -----Original Message----- > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > Sent: Friday, March 16, 2012 1:46 AM > > To: KY Srinivasan > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; ohering@xxxxxxxx; > > 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: > > > /* > > > * 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, > > > - (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
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
