From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v4 07/33] xen: guestcopy: Provide an helper
 to safely copy string from guest
Date: Tue, 31 Mar 2015 14:30:13 +0100
Message-ID: <551AA165.8090301@linaro.org>
References: <1426793399-6283-1-git-send-email-julien.grall@linaro.org>	<1426793399-6283-8-git-send-email-julien.grall@linaro.org>
	<551AA000.6090603@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <551AA000.6090603@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

Hi Andrew,

On 31/03/15 14:24, Andrew Cooper wrote:
>> +/*
>> + * The function copies a string from the guest and adds a NUL to
>> + * make sure the string is correctly terminated.
>> + */
>> +void *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
>> +                                  size_t size, size_t max_size)
> 
> If it is a NUL terminated string, you should return char* rather than void*.

Ok.

> Furthermore, two size parameters serves no useful purpose.  The caller
> must always be in a position to decide a plausible upper bound.

I don't understand the problem to have two size parameters...

The first one is the size given by the guest while the second one if the
upper bound.

The maximum size may change from every caller. Hence the second size
parameter.

>> +{
>> +    char *tmp;
>> +
>> +    if ( size > max_size )
>> +        return ERR_PTR(-ENOBUFS);
>> +
>> +    /* Add an extra +1 to append \0 */
>> +    tmp = xmalloc_array(char, size + 1);
> 
> Need to check that size + 1 doesn't overflow to 0.

With the max_size controlled by the caller there is no need to check the
overflow.

If the caller decides to give a too high max_size then it's its problem.

> 
>> +    if ( !tmp )
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    if ( copy_from_guest(tmp, u_buf, size) )
>> +    {
>> +        xfree(tmp);
>> +        return ERR_PTR(-EFAULT);
>> +    }
>> +    tmp[size] = 0;
> 
> '\0' please.

Ok.

Regards,

-- 
Julien Grall