[PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Jesper Juhl]

At the start of the function we assign 'a->d' to 'ap'. Then we use the
RESIZE_IF_NEEDED macro on 'a' - this may free 'a->d' and replace it
with newly allocaetd storage. In that case, we'll be operating on
freed memory further down in the function when we index into 'ap[]'.
Since we don't actually need 'ap' until after the use of the
RESIZE_IF_NEEDED macro we can just delay the assignment to it until
after we've potentially resized, thus avoiding the issue.

While I was there anyway I also changed the integer variable 'n' to be
const. It might as well be since we only assign to it once and use it
as a constant, and then the compiler will tell us if we ever assign to
it in the future.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 lib/mpi/mpi-bit.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

 only compile tested.

diff --git a/lib/mpi/mpi-bit.c b/lib/mpi/mpi-bit.c
index 2f52662..0c50536 100644
--- a/lib/mpi/mpi-bit.c
+++ b/lib/mpi/mpi-bit.c
@@ -177,8 +177,8 @@ int mpi_rshift(MPI x, MPI a, unsigned n)
  */
 int mpi_lshift_limbs(MPI a, unsigned int count)
 {
-	mpi_ptr_t ap = a->d;
-	int n = a->nlimbs;
+	const int n = a->nlimbs;
+	mpi_ptr_t ap;
 	int i;
 
 	if (!count || !n)
@@ -187,6 +187,7 @@ int mpi_lshift_limbs(MPI a, unsigned int count)
 	if (RESIZE_IF_NEEDED(a, n + count) < 0)
 		return -ENOMEM;
 
+	ap = a->d;
 	for (i = n - 1; i >= 0; i--)
 		ap[i + count] = ap[i];
 	for (i = 0; i < count; i++)
-- 
1.7.9


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Jesper Juhl]

Ping?

On Mon, 6 Feb 2012, Jesper Juhl wrote:

> At the start of the function we assign 'a->d' to 'ap'. Then we use the
> RESIZE_IF_NEEDED macro on 'a' - this may free 'a->d' and replace it
> with newly allocaetd storage. In that case, we'll be operating on
> freed memory further down in the function when we index into 'ap[]'.
> Since we don't actually need 'ap' until after the use of the
> RESIZE_IF_NEEDED macro we can just delay the assignment to it until
> after we've potentially resized, thus avoiding the issue.
> 
> While I was there anyway I also changed the integer variable 'n' to be
> const. It might as well be since we only assign to it once and use it
> as a constant, and then the compiler will tell us if we ever assign to
> it in the future.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  lib/mpi/mpi-bit.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
>  only compile tested.
> 
> diff --git a/lib/mpi/mpi-bit.c b/lib/mpi/mpi-bit.c
> index 2f52662..0c50536 100644
> --- a/lib/mpi/mpi-bit.c
> +++ b/lib/mpi/mpi-bit.c
> @@ -177,8 +177,8 @@ int mpi_rshift(MPI x, MPI a, unsigned n)
>   */
>  int mpi_lshift_limbs(MPI a, unsigned int count)
>  {
> -	mpi_ptr_t ap = a->d;
> -	int n = a->nlimbs;
> +	const int n = a->nlimbs;
> +	mpi_ptr_t ap;
>  	int i;
>  
>  	if (!count || !n)
> @@ -187,6 +187,7 @@ int mpi_lshift_limbs(MPI a, unsigned int count)
>  	if (RESIZE_IF_NEEDED(a, n + count) < 0)
>  		return -ENOMEM;
>  
> +	ap = a->d;
>  	for (i = n - 1; i >= 0; i--)
>  		ap[i + count] = ap[i];
>  	for (i = 0; i < count; i++)
> 

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Kasatkin, Dmitry]

On Thu, Mar 1, 2012 at 11:33 PM, Jesper Juhl <jj@chaosbits.net> wrote:
>
> Ping?
>
> On Mon, 6 Feb 2012, Jesper Juhl wrote:
>
>> At the start of the function we assign 'a->d' to 'ap'. Then we use the
>> RESIZE_IF_NEEDED macro on 'a' - this may free 'a->d' and replace it
>> with newly allocaetd storage. In that case, we'll be operating on
>> freed memory further down in the function when we index into 'ap[]'.
>> Since we don't actually need 'ap' until after the use of the
>> RESIZE_IF_NEEDED macro we can just delay the assignment to it until
>> after we've potentially resized, thus avoiding the issue.
>>
>> While I was there anyway I also changed the integer variable 'n' to be
>> const. It might as well be since we only assign to it once and use it
>> as a constant, and then the compiler will tell us if we ever assign to
>> it in the future.
>>
>> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
>> ---
>>  lib/mpi/mpi-bit.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>>  only compile tested.
>>
>> diff --git a/lib/mpi/mpi-bit.c b/lib/mpi/mpi-bit.c
>> index 2f52662..0c50536 100644
>> --- a/lib/mpi/mpi-bit.c
>> +++ b/lib/mpi/mpi-bit.c
>> @@ -177,8 +177,8 @@ int mpi_rshift(MPI x, MPI a, unsigned n)
>>   */
>>  int mpi_lshift_limbs(MPI a, unsigned int count)
>>  {
>> -     mpi_ptr_t ap = a->d;
>> -     int n = a->nlimbs;
>> +     const int n = a->nlimbs;
>> +     mpi_ptr_t ap;
>>       int i;
>>
>>       if (!count || !n)
>> @@ -187,6 +187,7 @@ int mpi_lshift_limbs(MPI a, unsigned int count)
>>       if (RESIZE_IF_NEEDED(a, n + count) < 0)
>>               return -ENOMEM;
>>
>> +     ap = a->d;
>>       for (i = n - 1; i >= 0; i--)
>>               ap[i + count] = ap[i];
>>       for (i = 0; i < count; i++)
>>
>
> --
> Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
> Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> Plain text mails only, please.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Hello,

I was travelling last week.
I will review the patch now..

Thanks,
Dmitry

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Jesper Juhl]

On Tue, 13 Mar 2012, Kasatkin, Dmitry wrote:

> On Thu, Mar 1, 2012 at 11:33 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> >
> > Ping?
> >
> > On Mon, 6 Feb 2012, Jesper Juhl wrote:
> >
> >> At the start of the function we assign 'a->d' to 'ap'. Then we use the
> >> RESIZE_IF_NEEDED macro on 'a' - this may free 'a->d' and replace it
> >> with newly allocaetd storage. In that case, we'll be operating on
> >> freed memory further down in the function when we index into 'ap[]'.
> >> Since we don't actually need 'ap' until after the use of the
> >> RESIZE_IF_NEEDED macro we can just delay the assignment to it until
> >> after we've potentially resized, thus avoiding the issue.
> >>
> >> While I was there anyway I also changed the integer variable 'n' to be
> >> const. It might as well be since we only assign to it once and use it
> >> as a constant, and then the compiler will tell us if we ever assign to
> >> it in the future.
> >>
> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> >> ---
[...]
> Hello,
> 
> I was travelling last week.
> I will review the patch now..
> 

:-) Thanks. No rush, I'd just like to see the fix included at some point 
if you agree that it is correct - just wondered at the "~1 month no 
response".


> Thanks,
> Dmitry
> 

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Jesper Juhl]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2809 bytes --]

On Tue, 13 Mar 2012, Kasatkin, Dmitry wrote:

> On Thu, Mar 1, 2012 at 11:33 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> >
> > Ping?
> >
> > On Mon, 6 Feb 2012, Jesper Juhl wrote:
> >
> >> At the start of the function we assign 'a->d' to 'ap'. Then we use the
> >> RESIZE_IF_NEEDED macro on 'a' - this may free 'a->d' and replace it
> >> with newly allocaetd storage. In that case, we'll be operating on
> >> freed memory further down in the function when we index into 'ap[]'.
> >> Since we don't actually need 'ap' until after the use of the
> >> RESIZE_IF_NEEDED macro we can just delay the assignment to it until
> >> after we've potentially resized, thus avoiding the issue.
> >>
> >> While I was there anyway I also changed the integer variable 'n' to be
> >> const. It might as well be since we only assign to it once and use it
> >> as a constant, and then the compiler will tell us if we ever assign to
> >> it in the future.
> >>
> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> >> ---
> >>  lib/mpi/mpi-bit.c |    5 +++--
> >>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >>  only compile tested.
> >>
> >> diff --git a/lib/mpi/mpi-bit.c b/lib/mpi/mpi-bit.c
> >> index 2f52662..0c50536 100644
> >> --- a/lib/mpi/mpi-bit.c
> >> +++ b/lib/mpi/mpi-bit.c
> >> @@ -177,8 +177,8 @@ int mpi_rshift(MPI x, MPI a, unsigned n)
> >>   */
> >>  int mpi_lshift_limbs(MPI a, unsigned int count)
> >>  {
> >> -     mpi_ptr_t ap = a->d;
> >> -     int n = a->nlimbs;
> >> +     const int n = a->nlimbs;
> >> +     mpi_ptr_t ap;
> >>       int i;
> >>
> >>       if (!count || !n)
> >> @@ -187,6 +187,7 @@ int mpi_lshift_limbs(MPI a, unsigned int count)
> >>       if (RESIZE_IF_NEEDED(a, n + count) < 0)
> >>               return -ENOMEM;
> >>
> >> +     ap = a->d;
> >>       for (i = n - 1; i >= 0; i--)
> >>               ap[i + count] = ap[i];
> >>       for (i = 0; i < count; i++)
> >>
> >
> > --
> > Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
> > Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> > Plain text mails only, please.
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> Hello,
> 
> I was travelling last week.
> I will review the patch now..
> 

Hi.

Did you get a chance to review the patch?  Did you find it acceptable or 
flawed?

If it is OK, what do I need to from this point forward to get it merged?


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Kasatkin, Dmitry]

On Thu, Apr 12, 2012 at 1:31 AM, Jesper Juhl <jj@chaosbits.net> wrote:
> On Tue, 13 Mar 2012, Kasatkin, Dmitry wrote:
>
>> On Thu, Mar 1, 2012 at 11:33 PM, Jesper Juhl <jj@chaosbits.net> wrote:
>> >
>> > Ping?
>> >
>> > On Mon, 6 Feb 2012, Jesper Juhl wrote:
>> >
>> >> At the start of the function we assign 'a->d' to 'ap'. Then we use the
>> >> RESIZE_IF_NEEDED macro on 'a' - this may free 'a->d' and replace it
>> >> with newly allocaetd storage. In that case, we'll be operating on
>> >> freed memory further down in the function when we index into 'ap[]'.
>> >> Since we don't actually need 'ap' until after the use of the
>> >> RESIZE_IF_NEEDED macro we can just delay the assignment to it until
>> >> after we've potentially resized, thus avoiding the issue.
>> >>
>> >> While I was there anyway I also changed the integer variable 'n' to be
>> >> const. It might as well be since we only assign to it once and use it
>> >> as a constant, and then the compiler will tell us if we ever assign to
>> >> it in the future.
>> >>
>> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
>> >> ---
>> >>  lib/mpi/mpi-bit.c |    5 +++--
>> >>  1 files changed, 3 insertions(+), 2 deletions(-)
>> >>
>> >>  only compile tested.
>> >>
>> >> diff --git a/lib/mpi/mpi-bit.c b/lib/mpi/mpi-bit.c
>> >> index 2f52662..0c50536 100644
>> >> --- a/lib/mpi/mpi-bit.c
>> >> +++ b/lib/mpi/mpi-bit.c
>> >> @@ -177,8 +177,8 @@ int mpi_rshift(MPI x, MPI a, unsigned n)
>> >>   */
>> >>  int mpi_lshift_limbs(MPI a, unsigned int count)
>> >>  {
>> >> -     mpi_ptr_t ap = a->d;
>> >> -     int n = a->nlimbs;
>> >> +     const int n = a->nlimbs;
>> >> +     mpi_ptr_t ap;
>> >>       int i;
>> >>
>> >>       if (!count || !n)
>> >> @@ -187,6 +187,7 @@ int mpi_lshift_limbs(MPI a, unsigned int count)
>> >>       if (RESIZE_IF_NEEDED(a, n + count) < 0)
>> >>               return -ENOMEM;
>> >>
>> >> +     ap = a->d;
>> >>       for (i = n - 1; i >= 0; i--)
>> >>               ap[i + count] = ap[i];
>> >>       for (i = 0; i < count; i++)
>> >>
>> >
>> > --
>> > Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
>> > Don't top-post http://www.catb.org/jargon/html/T/top-post.html
>> > Plain text mails only, please.
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>>
>> Hello,
>>
>> I was travelling last week.
>> I will review the patch now..
>>
>
> Hi.
>
> Did you get a chance to review the patch?  Did you find it acceptable or
> flawed?
>
> If it is OK, what do I need to from this point forward to get it merged?
>
>

Hello Jesper & James,

That is of course correct.

James, can you please apply this patch to security fixes?

 Acked-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>

- Dmitry

> --
> Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
> Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> Plain text mails only, please.

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Jesper Juhl]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3277 bytes --]

On Thu, 12 Apr 2012, Kasatkin, Dmitry wrote:

> On Thu, Apr 12, 2012 at 1:31 AM, Jesper Juhl <jj@chaosbits.net> wrote:
> > On Tue, 13 Mar 2012, Kasatkin, Dmitry wrote:
> >
> >> On Thu, Mar 1, 2012 at 11:33 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> >> >
> >> > Ping?
> >> >
> >> > On Mon, 6 Feb 2012, Jesper Juhl wrote:
> >> >
> >> >> At the start of the function we assign 'a->d' to 'ap'. Then we use the
> >> >> RESIZE_IF_NEEDED macro on 'a' - this may free 'a->d' and replace it
> >> >> with newly allocaetd storage. In that case, we'll be operating on
> >> >> freed memory further down in the function when we index into 'ap[]'.
> >> >> Since we don't actually need 'ap' until after the use of the
> >> >> RESIZE_IF_NEEDED macro we can just delay the assignment to it until
> >> >> after we've potentially resized, thus avoiding the issue.
> >> >>
> >> >> While I was there anyway I also changed the integer variable 'n' to be
> >> >> const. It might as well be since we only assign to it once and use it
> >> >> as a constant, and then the compiler will tell us if we ever assign to
> >> >> it in the future.
> >> >>
> >> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> >> >> ---
> >> >>  lib/mpi/mpi-bit.c |    5 +++--
> >> >>  1 files changed, 3 insertions(+), 2 deletions(-)
> >> >>
> >> >>  only compile tested.
> >> >>
> >> >> diff --git a/lib/mpi/mpi-bit.c b/lib/mpi/mpi-bit.c
> >> >> index 2f52662..0c50536 100644
> >> >> --- a/lib/mpi/mpi-bit.c
> >> >> +++ b/lib/mpi/mpi-bit.c
> >> >> @@ -177,8 +177,8 @@ int mpi_rshift(MPI x, MPI a, unsigned n)
> >> >>   */
> >> >>  int mpi_lshift_limbs(MPI a, unsigned int count)
> >> >>  {
> >> >> -     mpi_ptr_t ap = a->d;
> >> >> -     int n = a->nlimbs;
> >> >> +     const int n = a->nlimbs;
> >> >> +     mpi_ptr_t ap;
> >> >>       int i;
> >> >>
> >> >>       if (!count || !n)
> >> >> @@ -187,6 +187,7 @@ int mpi_lshift_limbs(MPI a, unsigned int count)
> >> >>       if (RESIZE_IF_NEEDED(a, n + count) < 0)
> >> >>               return -ENOMEM;
> >> >>
> >> >> +     ap = a->d;
> >> >>       for (i = n - 1; i >= 0; i--)
> >> >>               ap[i + count] = ap[i];
> >> >>       for (i = 0; i < count; i++)
> >> >>
> >> >
> >> > --
> >> > Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
> >> > Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> >> > Plain text mails only, please.
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> > Please read the FAQ at  http://www.tux.org/lkml/
> >>
> >> Hello,
> >>
> >> I was travelling last week.
> >> I will review the patch now..
> >>
> >
> > Hi.
> >
> > Did you get a chance to review the patch?  Did you find it acceptable or
> > flawed?
> >
> > If it is OK, what do I need to from this point forward to get it merged?
> >
> >
> 
> Hello Jesper & James,
> 
> That is of course correct.
> 
> James, can you please apply this patch to security fixes?
> 
>  Acked-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> 

Thank you for your time to review and your ACK :-)

-- 
Jesper Juhl <jj@chaosbits.net>

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[James Morris]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 488 bytes --]

On Thu, 12 Apr 2012, Kasatkin, Dmitry wrote:

> >> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> >> >> ---
> >> >>  lib/mpi/mpi-bit.c |    5 +++--
> >> >>  1 files changed, 3 insertions(+), 2 deletions(-)
> >> >>
> >> >>  only compile tested.

> That is of course correct.
> 
> James, can you please apply this patch to security fixes?
> 
>  Acked-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> 
> - Dmitry

Has this patch been tested?



-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Jesper Juhl]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 822 bytes --]

On Sat, 14 Apr 2012, James Morris wrote:

> On Thu, 12 Apr 2012, Kasatkin, Dmitry wrote:
> 
> > >> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > >> >> ---
> > >> >>  lib/mpi/mpi-bit.c |    5 +++--
> > >> >>  1 files changed, 3 insertions(+), 2 deletions(-)
> > >> >>
> > >> >>  only compile tested.
> 
> > That is of course correct.
> > 
> > James, can you please apply this patch to security fixes?
> > 
> >  Acked-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > 
> > - Dmitry
> 
> Has this patch been tested?
> 
Personally I don't really have a way to test it properly, so the only 
testing I've personally done is to check that it compiles.

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] mpi: Avoid using freed pointer in mpi_lshift_limbs()

[Kasatkin, Dmitry]

On Sat, Apr 14, 2012 at 11:32 PM, Jesper Juhl <jj@chaosbits.net> wrote:
> On Sat, 14 Apr 2012, James Morris wrote:
>
>> On Thu, 12 Apr 2012, Kasatkin, Dmitry wrote:
>>
>> > >> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
>> > >> >> ---
>> > >> >>  lib/mpi/mpi-bit.c |    5 +++--
>> > >> >>  1 files changed, 3 insertions(+), 2 deletions(-)
>> > >> >>
>> > >> >>  only compile tested.
>>
>> > That is of course correct.
>> >
>> > James, can you please apply this patch to security fixes?
>> >
>> >  Acked-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
>> >
>> > - Dmitry
>>
>> Has this patch been tested?
>>
> Personally I don't really have a way to test it properly, so the only
> testing I've personally done is to check that it compiles.
>

It works...
In fact there are few other places in the code which take pointer
after re-sizing...

- Dmitry

> --
> Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
> Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> Plain text mails only, please.