Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

From: Dan Carpenter <dan.carpenter@oracle.com>
To: KY Srinivasan <kys@microsoft.com>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"ohering@suse.com" <ohering@suse.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"virtualization@lists.osdl.org" <virtualization@lists.osdl.org>,
	Alan Stern <stern@rowland.harvard.edu>,
	"devel@linuxdriverproject.org" <devel@linuxdriverproject.org>
Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
Date: Fri, 16 Mar 2012 10:37:22 +0300	[thread overview]
Message-ID: <20120316073722.GI3163@mwanda> (raw)
In-Reply-To: <20120316071858.GH3220@mwanda>

[-- Attachment #1.1: Type: text/plain, Size: 2599 bytes --]

On Fri, Mar 16, 2012 at 10:18:58AM +0300, Dan Carpenter wrote:
> > > 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.
> 

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel