Re: [PATCH] staging: tidspbridge: fix signess error

Re: [PATCH] staging: tidspbridge: fix signess error

[Dan Rosenberg]

It seems to me that in a lot of cases, signed types were used initially
with checks that the value was not below zero, but then unsigned types
were substituted due to problems later on (potentially security
related), and the checks were never removed.  Just because a check
doesn't make sense doesn't necessarily mean it's safe to revert an
unsigned type to a signed one - just be careful to make sure you're not
introducing any problems when you make these kinds of changes.  I have
no idea whether or not this is the case in this particular example.

> i was unsigned, so check for (i < 0) made no sense.  Made it signed.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> ---
>  I cannot compile this driver, so it is not tested.
>  drivers/staging/tidspbridge/core/dsp-mmu.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
> index 3c978f9..983c95a 100644
> --- a/drivers/staging/tidspbridge/core/dsp-mmu.c
> +++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
> @@ -219,7 +219,8 @@ u32 user_to_dsp_map(struct iommu *mmu, u32 uva, u32 da, u32 size,
>  				struct page **usr_pgs)
>  {
>  	int res, w;
> -	unsigned pages, i;
> +	unsigned pages;
> +	int i;
>  	struct vm_area_struct *vma;
>  	struct mm_struct *mm = current->mm;
>  	struct sg_table *sgt;
> -- 
> 1.7.0.4

Re: [PATCH] staging: tidspbridge: fix signess error

[Vasiliy Kulikov]

On Mon, Oct 11, 2010 at 16:24 -0400, Dan Rosenberg wrote:
> It seems to me that in a lot of cases, signed types were used initially
> with checks that the value was not below zero, but then unsigned types
> were substituted due to problems later on (potentially security
> related), and the checks were never removed.  Just because a check
> doesn't make sense doesn't necessarily mean it's safe to revert an
> unsigned type to a signed one - just be careful to make sure you're not
> introducing any problems when you make these kinds of changes.  I have
> no idea whether or not this is the case in this particular example.

i is used as the store for get_user_pages() result, which may be
-EFAULT.

> > i was unsigned, so check for (i < 0) made no sense.  Made it signed.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> > ---
> >  I cannot compile this driver, so it is not tested.
> >  drivers/staging/tidspbridge/core/dsp-mmu.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
> > index 3c978f9..983c95a 100644
> > --- a/drivers/staging/tidspbridge/core/dsp-mmu.c
> > +++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
> > @@ -219,7 +219,8 @@ u32 user_to_dsp_map(struct iommu *mmu, u32 uva, u32 da, u32 size,
> >  				struct page **usr_pgs)
> >  {
> >  	int res, w;
> > -	unsigned pages, i;
> > +	unsigned pages;
> > +	int i;
> >  	struct vm_area_struct *vma;
> >  	struct mm_struct *mm = current->mm;
> >  	struct sg_table *sgt;
> > -- 
> > 1.7.0.4
> 
> 

-- 
Vasiliy

[PATCH] staging: tidspbridge: fix signess error

[Vasiliy Kulikov]

i was unsigned, so check for (i < 0) made no sense.  Made it signed.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 I cannot compile this driver, so it is not tested.

 drivers/staging/tidspbridge/core/dsp-mmu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 3c978f9..983c95a 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -219,7 +219,8 @@ u32 user_to_dsp_map(struct iommu *mmu, u32 uva, u32 da, u32 size,
 				struct page **usr_pgs)
 {
 	int res, w;
-	unsigned pages, i;
+	unsigned pages;
+	int i;
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 	struct sg_table *sgt;
-- 
1.7.0.4