From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 07/33] xen: guestcopy: Provide an helper to safely copy string from guest Date: Tue, 31 Mar 2015 15:00:16 +0100 Message-ID: <551AA870.4090502@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> <551AA165.8090301@linaro.org> <551AA5FF.5090509@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <551AA5FF.5090509@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 31/03/15 14:49, Andrew Cooper wrote: > On 31/03/15 14:30, Julien Grall wrote: >> >>> 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. > > The caller shouldn't even be calling safe_copy_string_from_guest() with > a guest-controlled-implausibly-large size. > > The caller should be doing something like: > > if ( usersize > PLAUSIBLE_UPPER_BOUND ) > ... fail > else > data = safe_copy_string_from_guest(hnd, usersize). > > > Mixing plausibility checks and string copying in a single function is a > antipattern, and IMO should not be moved into a common helper function > like this. Why it's an antipattern? It's exactly the same as checking the validity of the buffer in copy_from_guest... safe_copy-string_from_guest will fail if the size is too high. Caller of this function may forget to do the check and introduce a security issue. Having the check in safe_copy_string_from_guest avoid this problem. Regards, -- Julien Grall