From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.9030.1675427271662559734 for ; Fri, 03 Feb 2023 04:27:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=U8OKkTt+; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id A580EB82AA7 for ; Fri, 3 Feb 2023 12:27:49 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E32A4C433AF for ; Fri, 3 Feb 2023 12:27:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675427267; bh=ShkUnZbNHZKoegMeqQriI5NHCKe8iMh74k1+BP2K/ZQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=U8OKkTt+jPvrTIr7rGqYmy4Ltupq3Km6hGkbvzDWoZhyVkKsIHWAEmqNUusuWvbLo EnZJC6APBe38BuExs3Qxh3fF5VharGKInL+bUClNZQWJ5rTYqwUYr7/PfdVvGqu1m3 wUbCoitTwibB7zJHJ5La1gVHbZho1lj/dcYKeSwqMHNEuHtm+XUQ9a/s6fUbidIw6R /o/Qxgi983vr/LhXgmxj/65a1pNTU/laPKhlYZTIzBPoHqMdVRTEMXc7VOWQLaoA6K sxwp0M2/WlcsYO9cwRZ+HZcGKfj6Q5wi0b7XKSFkfhsYvN2SOXtABIR1Sh5jbMMIRY 1I1S1GvGIkCcw== Received: by mail-lf1-f49.google.com with SMTP id v17so7547180lfd.7 for ; Fri, 03 Feb 2023 04:27:47 -0800 (PST) X-Gm-Message-State: AO0yUKWaORLjRBew/q9OapgFxUlWWC+qyJJtYCnc/ObOcnnGYppbu1Kv 78OBDk3ugxQVvb+OMOX0iE0QiuqwRkbAhhObczw= X-Google-Smtp-Source: AK7set9YNrrJEGJeve/sjobLLxDHIihycl99tGt7cA65v5mu7U3CaSddjUlJ+7qLVJAKhwXMIUESTrBYZUCN3yK2CUY= X-Received: by 2002:ac2:53b3:0:b0:4b6:e197:3aeb with SMTP id j19-20020ac253b3000000b004b6e1973aebmr1964791lfh.233.1675427265934; Fri, 03 Feb 2023 04:27:45 -0800 (PST) MIME-Version: 1.0 References: <20230202143015.3066988-1-sudeep.holla@arm.com> <20230203112003.fstpmkec5w7zcq6z@bogus> In-Reply-To: <20230203112003.fstpmkec5w7zcq6z@bogus> From: "Ard Biesheuvel" Date: Fri, 3 Feb 2023 13:27:34 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] ArmPkg/ArmScmiDxe: Fix the calculation of RequiredArraySize in ClockDescribeRates() To: devel@edk2.groups.io, sudeep.holla@arm.com Cc: Pierre Gondois , Leif Lindholm , Ard Biesheuvel , Girish Pathak , Jeff Brasen , Sami Mujawar Content-Type: text/plain; charset="UTF-8" On Fri, 3 Feb 2023 at 12:20, Sudeep Holla wrote: > > On Fri, Feb 03, 2023 at 12:10:38PM +0100, Pierre Gondois wrote: > > Hello Sudeep, > > > > On 2/2/23 15:30, Sudeep Holla wrote: > > > As per the SCMI specification, section CLOCK_DESCRIBE_RATES mentions that > > > the value of num_rates_flags[11:0] in the response must be 3 if the return > > > format is the triplet. Due to the buggy firmware, this was not noticed for > > > long time. The firmware is now fixed resulting in ClockDescribeRates() to > > > fail with "Buffer Too Small" error as the RequiredArraySize gets miscalculated > > > as 72 instead of 24. > > > > > > Fix the issue by reusing the logic for both the return format which must > > > work if num_rates_flags has correct value as expected from the specification. > > > > > > Cc: Pierre Gondois > > > Cc: Girish Pathak > > > Cc: Jeff Brasen > > > Reported-by: Sami Mujawar > > > Signed-off-by: Sudeep Holla > > > --- > > > ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c | 7 +------ > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > Hi All, > > > > > > Sorry for the resend, I hadn't subscribed before so resending after the > > > subscription. > > > > > > Regards, > > > Sudeep > > > > > > diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c > > > index 12a7e6df5d43..1412717bc227 100644 > > > --- a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c > > > +++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c > > > @@ -236,12 +236,7 @@ ClockDescribeRates ( > > > *TotalRates = NUM_RATES (DescribeRates->NumRatesFlags) > > > + NUM_REMAIN_RATES (DescribeRates->NumRatesFlags); > > > - if (*Format == ScmiClockRateFormatDiscrete) { > > > - RequiredArraySize = (*TotalRates) * sizeof (UINT64); > > > - } else { > > > - // We need to return triplet of 64 bit value for each rate > > > - RequiredArraySize = (*TotalRates) * 3 * sizeof (UINT64); > > > - } > > > + RequiredArraySize = (*TotalRates) * sizeof (UINT64); > > > if (RequiredArraySize > (*RateArraySize)) { > > > *RateArraySize = RequiredArraySize; > > > > I think the following also needs to be modified: > > """ > > for (RateNo = 0; RateNo < NUM_RATES (DescribeRates->NumRatesFlags); RateNo++) { > > // Linear clock rates from minimum to maximum in steps > > // Minimum clock rate. > > Rate = &DescribeRates->Rates[RateOffset++]; > > RateArray[RateIndex].ContinuousRate.Min = > > ConvertTo64Bit (Rate->Low, Rate->High); > > > > Rate = &DescribeRates->Rates[RateOffset++]; > > // Maximum clock rate. > > RateArray[RateIndex].ContinuousRate.Max = > > ConvertTo64Bit (Rate->Low, Rate->High); > > > > Rate = &DescribeRates->Rates[RateOffset++]; > > // Step. > > RateArray[RateIndex++].ContinuousRate.Step = > > ConvertTo64Bit (Rate->Low, Rate->High); > > } > > """ > > > > Right, good catch. For some reason, I don't see any exceptions in release > build :(. I will try the debug build and check. Thanks for the review. > > > > With the above loop removed: > > Reviewed-by: Pierre Gondois > > Tested-by: Pierre Gondois > > > > Thanks! > Excellent. Could someone post the final version please?