Message-ID: <6E21E5352C11B742B20C142EB499E0481B76671F@TK5EX14MBXC126.redmond.corp.microsoft.com>
Date:	Fri, 16 Mar 2012 13:14:07 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"virtualization@...ts.osdl.org" <virtualization@...ts.osdl.org>,
	"ohering@...e.com" <ohering@...e.com>,
	Alan Stern <stern@...land.harvard.edu>
Subject: RE: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
 messages in the driver



> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@...cle.com]
> Sent: Friday, March 16, 2012 3:19 AM
> To: KY Srinivasan
> Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org;
> devel@...uxdriverproject.org; virtualization@...ts.osdl.org; ohering@...e.com;
> Alan Stern
> Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@...cle.com]
> > > Sent: Friday, March 16, 2012 1:46 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org;
> > > devel@...uxdriverproject.org; virtualization@...ts.osdl.org;
> ohering@...e.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:
> > > >  	/*
> > > >  	 * 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.

Dan,
I am trying to understand your concern here:
You will agree with me that the current code does not overflow the 
buffer since the output is limited to MAX bytes and that is how 
big the output buffer is sized. Now, as I pointed out earlier, if the
string to be converted were to fully occupy the MAX bytes or even be
larger than MAX bytes (no buffer overflow here since we limit the
conversion to MAX bytes), we will get a malformed packet that will be
sent to the host since the size field of the message would exceed
the protocol specified size limit. I was merely pointing out that in
this case, I would rather have the host reject the message than send
a truncated Key/Value string (if we were to ever get invalid key or
value strings). 

> 
> 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. */.
> 
All I am saying if we have an invalid string we will (a) ensure that we don't overflow
the output buffer and (b) we will generate a malformed packet as we should since
the string we are handling is not valid. Furthermore, I am suggesting that it is better
to have a malformed packet in this case than to have 

> > 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.

The host here is a windows host and the protocol verification engine there
will reject a mal-formed message. Having said this, what you are 
proposing is reasonable, I will re-send the patches.

Regards,

K. Y





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists