From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yizhuo Zhai Subject: Re: [patch] /drivers/iommu/amd_iommu.c: variable unmap_size could be uninitialized Date: Thu, 31 May 2018 11:21:48 -0700 Message-ID: References: <20180531094415.GR18595@8bytes.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2000442818723583208==" Return-path: In-Reply-To: <20180531094415.GR18595-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Joerg Roedel Cc: Chengyu Song , iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Zhiyun Qian , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: iommu@lists.linux-foundation.org --===============2000442818723583208== Content-Type: multipart/alternative; boundary="000000000000671fa5056d848a0b" --000000000000671fa5056d848a0b Content-Type: text/plain; charset="UTF-8" Yes, thank you for your advice. The new patch's been sent. On Thu, May 31, 2018 at 2:44 AM, Joerg Roedel wrote: > Hi Yizhuo Zhai, > > thanks for your patch, but I think there is a better way to fix that. > Please see below. > > On Wed, May 30, 2018 at 11:02:54PM -0700, Yizhuo Zhai wrote: > > Variable "unmap_size" is supposed to be initialized in function > fetch_pte. > > However, it's uninitialized if fetch_pte returns NULL. And "unmap_size" > is used > > outside the return check. > > > > From 377ccb647d3c6c6747f20a242b035bafc775c3be Mon Sep 17 00:00:00 2001 > > > > Signed-off-by: From: "yzhai003-3vSkeFsW7jA@public.gmane.org" > > --- > > drivers/iommu/amd_iommu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c > > index 8fb8c73..774e057 100644 > > --- a/drivers/iommu/amd_iommu.c > > +++ b/drivers/iommu/amd_iommu.c > > @@ -1500,7 +1500,7 @@ static unsigned long iommu_unmap_page(struct > > protection_domain *dom, > > unsigned long page_size) > > { > > unsigned long long unmapped; > > - unsigned long unmap_size; > > + unsigned long unmap_size = 0; > > That is not sufficient because fetch_pte is called in a loop, and when > it returns NULL then unmap_size keeps the value of the previous > invocation. > > So from looking at the code, it is better to set page_size=0 in > fetch_pte at the very beginning, before the function can return NULL. > > > Thanks, > > Joerg > > -- Kind Regards, *Yizhuo Zhai* *Computer Science, Graduate Student* *University of California, Riverside * --000000000000671fa5056d848a0b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Yes, thank you for your advice. The new patch's been s= ent.

On Thu,= May 31, 2018 at 2:44 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote= :
Hi Yizhuo Zhai,

thanks for your patch, but I think there is a better way to fix that.
Please see below.

On Wed, May 30, 2018 at 11:02:54PM -0700, Yizhuo Zhai wrote:
> Variable "unmap_size" is supposed to be initialized in funct= ion fetch_pte.
> However, it's uninitialized if fetch_pte returns NULL. And "u= nmap_size" is used
> outside the return check.=C2=A0
>
> From 377ccb647d3c6c6747f20a242b035bafc775c3be Mon Sep 17 00:00:00= 2001
>
> Signed-off-by: From: "yzhai00= 3@ucr.edu" <yzhai003-Kj4AhJ8wZtA@public.gmane.org= u>
> ---
> =C2=A0drivers/iommu/amd_iommu.c | 2 +-
> =C2=A01 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 8fb8c73..774e057 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1500,7 +1500,7 @@ static unsigned long iommu_unmap_page(struct
> protection_domain *dom,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long page_size)
> =C2=A0{
> =C2=A0 unsigned long long unmapped;
> - unsigned long unmap_size;
> + unsigned long unmap_size =3D 0;

That is not sufficient because fetch_pte is called in a loop, and wh= en
it returns NULL then unmap_size keeps the value of the previous
invocation.

So from looking at the code, it is better to set page_size=3D0 in
fetch_pte at the very beginning, before the function can return NULL.


Thanks,

=C2=A0 =C2=A0 =C2=A0 =C2=A0 Joerg




--
Kind Regards,

Yizh= uo Zhai

Computer Science, Graduate Student
University of California, Riverside=C2=A0=
--000000000000671fa5056d848a0b-- --===============2000442818723583208== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============2000442818723583208==--