[PATCHv2] x86/crtools: Fix null pointer dereference

Submitted by Radostin Stoyanov on May 2, 2019, 9:36 a.m.

Details

Message ID 20190502093640.9959-1-rstoyanov1@gmail.com
State New
Series "x86/crtools: Fix null pointer dereference"
Headers show

Commit Message

Radostin Stoyanov May 2, 2019, 9:36 a.m.
Dereferencing a null pointer is undefined behavior.

ISO/IEC 9899, clause 6.5.3.2, paragraph 4
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf

v2: declare x with always unused attribute. (thanks Cyrill Gorcunov)

Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
---
 criu/arch/x86/crtools.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/arch/x86/crtools.c b/criu/arch/x86/crtools.c
index ee016da00..865efc513 100644
--- a/criu/arch/x86/crtools.c
+++ b/criu/arch/x86/crtools.c
@@ -288,21 +288,21 @@  void arch_free_thread_info(CoreEntry *core)
 static bool valid_xsave_frame(CoreEntry *core)
 {
 	UserX86XsaveEntry *xsave = core->thread_info->fpregs->xsave;
-	struct xsave_struct *x = NULL;
+	struct xsave_struct __always_unused x;
 
-	if (core->thread_info->fpregs->n_st_space < ARRAY_SIZE(x->i387.st_space)) {
+	if (core->thread_info->fpregs->n_st_space < ARRAY_SIZE(x.i387.st_space)) {
 		pr_err("Corruption in FPU st_space area "
 		       "(got %li but %li expected)\n",
 		       (long)core->thread_info->fpregs->n_st_space,
-		       (long)ARRAY_SIZE(x->i387.st_space));
+		       (long)ARRAY_SIZE(x.i387.st_space));
 		return false;
 	}
 
-	if (core->thread_info->fpregs->n_xmm_space < ARRAY_SIZE(x->i387.xmm_space)) {
+	if (core->thread_info->fpregs->n_xmm_space < ARRAY_SIZE(x.i387.xmm_space)) {
 		pr_err("Corruption in FPU xmm_space area "
 		       "(got %li but %li expected)\n",
 		       (long)core->thread_info->fpregs->n_st_space,
-		       (long)ARRAY_SIZE(x->i387.xmm_space));
+		       (long)ARRAY_SIZE(x.i387.xmm_space));
 		return false;
 	}
 

Comments

Cyrill Gorcunov May 2, 2019, 10:08 a.m.
On Thu, May 02, 2019 at 10:36:40AM +0100, Radostin Stoyanov wrote:
> Dereferencing a null pointer is undefined behavior.
> 
> ISO/IEC 9899, clause 6.5.3.2, paragraph 4
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf
> 
> v2: declare x with always unused attribute. (thanks Cyrill Gorcunov)
> 
> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Thanks!
Dmitry Safonov May 2, 2019, 1:12 p.m.
On 5/2/19 10:36 AM, Radostin Stoyanov wrote:
> Dereferencing a null pointer is undefined behavior.
> 
> ISO/IEC 9899, clause 6.5.3.2, paragraph 4
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf

That doesn't make sense, sorry.

sizeof() operator doesn't evaluate expression as long as it's not a
var-array (which is not the case), check in the paper 6.5.3.4:
"If the type of the operand is a variable length array type, the operand
is evaluated; otherwise, the operand is not evaluated and the result is
an integer constant."

Basically, in this case it's a compile-time constant.
I.e.:
sizeof(valid_xsave_frame(NULL)) will be the same as sizeof(bool),
without actual runtime function call.

So, as I don't see any point in the change,

Nacked-by: Dmitry Safonov <0x7f454c46@gmail.com>

Though, there are those ugly casts to (long) which could be addressed
with %zu instead of %li modifier.

> 
> v2: declare x with always unused attribute. (thanks Cyrill Gorcunov)
> 
> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> ---
>  criu/arch/x86/crtools.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/criu/arch/x86/crtools.c b/criu/arch/x86/crtools.c
> index ee016da00..865efc513 100644
> --- a/criu/arch/x86/crtools.c
> +++ b/criu/arch/x86/crtools.c
> @@ -288,21 +288,21 @@ void arch_free_thread_info(CoreEntry *core)
>  static bool valid_xsave_frame(CoreEntry *core)
>  {
>  	UserX86XsaveEntry *xsave = core->thread_info->fpregs->xsave;
> -	struct xsave_struct *x = NULL;
> +	struct xsave_struct __always_unused x;
>  
> -	if (core->thread_info->fpregs->n_st_space < ARRAY_SIZE(x->i387.st_space)) {
> +	if (core->thread_info->fpregs->n_st_space < ARRAY_SIZE(x.i387.st_space)) {
>  		pr_err("Corruption in FPU st_space area "
>  		       "(got %li but %li expected)\n",
>  		       (long)core->thread_info->fpregs->n_st_space,
> -		       (long)ARRAY_SIZE(x->i387.st_space));
> +		       (long)ARRAY_SIZE(x.i387.st_space));
>  		return false;
>  	}
>  
> -	if (core->thread_info->fpregs->n_xmm_space < ARRAY_SIZE(x->i387.xmm_space)) {
> +	if (core->thread_info->fpregs->n_xmm_space < ARRAY_SIZE(x.i387.xmm_space)) {
>  		pr_err("Corruption in FPU xmm_space area "
>  		       "(got %li but %li expected)\n",
>  		       (long)core->thread_info->fpregs->n_st_space,
> -		       (long)ARRAY_SIZE(x->i387.xmm_space));
> +		       (long)ARRAY_SIZE(x.i387.xmm_space));
>  		return false;
>  	}
>  
> 

Thanks,
          Dmitry
Radostin Stoyanov May 2, 2019, 2:53 p.m.
On 02/05/2019 14:12, Dmitry Safonov wrote:
> On 5/2/19 10:36 AM, Radostin Stoyanov wrote:
>> Dereferencing a null pointer is undefined behavior.
>>
>> ISO/IEC 9899, clause 6.5.3.2, paragraph 4
>> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf
> That doesn't make sense, sorry.
>
> sizeof() operator doesn't evaluate expression as long as it's not a
> var-array (which is not the case), check in the paper 6.5.3.4:
> "If the type of the operand is a variable length array type, the operand
> is evaluated; otherwise, the operand is not evaluated and the result is
> an integer constant."
>
> Basically, in this case it's a compile-time constant.
> I.e.:
> sizeof(valid_xsave_frame(NULL)) will be the same as sizeof(bool),
> without actual runtime function call.
This is good to know, thank you for pointing it out.

Radostin
Dmitry Safonov May 2, 2019, 4:14 p.m.
On Thu, 2 May 2019 at 15:54, Radostin Stoyanov <rstoyanov1@gmail.com> wrote:
>
> On 02/05/2019 14:12, Dmitry Safonov wrote:
> > On 5/2/19 10:36 AM, Radostin Stoyanov wrote:
> >> Dereferencing a null pointer is undefined behavior.
> >>
> >> ISO/IEC 9899, clause 6.5.3.2, paragraph 4
> >> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf
> > That doesn't make sense, sorry.
> >
> > sizeof() operator doesn't evaluate expression as long as it's not a
> > var-array (which is not the case), check in the paper 6.5.3.4:
> > "If the type of the operand is a variable length array type, the operand
> > is evaluated; otherwise, the operand is not evaluated and the result is
> > an integer constant."
> >
> > Basically, in this case it's a compile-time constant.
> > I.e.:
> > sizeof(valid_xsave_frame(NULL)) will be the same as sizeof(bool),
> > without actual runtime function call.
> This is good to know, thank you for pointing it out.

No worries - I don't usually nack anything and don't like to do it,
hopefully, my reply wasn't harsh.

Thanks,
             Dmitry