Skip to contentSkip to conversation

Could info leak in preserve_iwmmxt_context() ?

PATCH Yang Yingliang 4 participants 5 messages Jul 9, 2019 – Jul 11, 2019 Arm (32-bit)

lore
Share & tools
lore

Yang Yingliang

author
#0 lore raw

Hi, Julien

In this commit 73839798af7e ("ARM: 8790/1: signal: always use __copy_to_user to save iwmmxt context"):

arch/arm/kernel/signal.c +4 -4
@@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe __user *frame)
         kframe->magic = IWMMXT_MAGIC;
         kframe->size = IWMMXT_STORAGE_SIZE;
         iwmmxt_task_copy(current_thread_info(), &kframe->storage);
-
-        err = __copy_to_user(frame, kframe, sizeof(*frame));
     } else {
         /*
          * For bug-compatibility with older kernels, some space

@@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe __user *frame)
          * Set the magic and size appropriately so that properly
          * written userspace can skip it reliably:
          */
-        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
-        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
+        *kframe = (struct iwmmxt_sigframe) {
+            .magic = DUMMY_MAGIC,
+            .size  = IWMMXT_STORAGE_SIZE,
+        };

The storage member of kframe is uninitialized, it seems will lead a info leak to userspace ?

In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it has no specific behavior to define the uninitialized member.

Please correct me if I am wrong.

     }

+ err = __copy_to_user(frame, kframe, sizeof(*kframe));
+

Thanks,
Yang

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#1 lore raw

Hi Yang,

Quoted 32 lines · Yang Yingliang

Hi, Julien

In this commit 73839798af7e ("ARM: 8790/1: signal: always use __copy_to_user to save iwmmxt context"):

arch/arm/kernel/signal.c +0 -2
@@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
          kframe->magic = IWMMXT_MAGIC;
          kframe->size = IWMMXT_STORAGE_SIZE;
          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
-
-        err = __copy_to_user(frame, kframe, sizeof(*frame));
      } else {
          /*

          * For bug-compatibility with older kernels, some space

arch/arm/kernel/signal.c +4 -2
@@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
           * Set the magic and size appropriately so that properly
           * written userspace can skip it reliably:
           */
-        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
-        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
+        *kframe = (struct iwmmxt_sigframe) {
+            .magic = DUMMY_MAGIC,
+            .size  = IWMMXT_STORAGE_SIZE,
+        };

The storage member of kframe is uninitialized, it seems will lead a info leak to userspace ?

Quoted 6 lines

In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it has no specific behavior
to define the uninitialized member.

Please correct me if I am wrong.

My understanding is that when using a compound initializer (either at variable declaration or by assigning a compound literal like in this case), the unspecified members get initialized to 0.

In the GNU-C section you mentioned [1] , there is an example:

    You can also initialize fewer than all of a structure variable’s
    members:

    struct pointy
      {
        int x, y;
        char *p;
      };
    struct pointy first_pointy = { 5 };

    Here, x is initialized with 5, y is initialized with 0, and p is
    initialized with NULL. The rule here is that y and p are initialized
    just as they would be if they were static variables.

So even when the manual refers to not initializing members, I think it just means that they are not explicitly initialized, i.e. by the developer. All the members of the structure still gets initialized to known values when doing an assignment to the whole structure.

One thing that Russell did mention was that initialization of padding bytes (that aren't part of a structure member but still within the structure's space) is unspecified. But in the case of iwmmxt_sigframe there is no padding.

[1]
https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members

Cheers,

Signature · 6 lines
Julien Thierry

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#2 lore raw
Quoted 39 lines · Julien Thierry

Hi Yang,

Quoted 36 lines · Yang Yingliang

Hi, Julien

In this commit 73839798af7e ("ARM: 8790/1: signal: always use __copy_to_user to save iwmmxt context"):

arch/arm/kernel/signal.c +0 -2
@@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
          kframe->magic = IWMMXT_MAGIC;
          kframe->size = IWMMXT_STORAGE_SIZE;
          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
-
-        err = __copy_to_user(frame, kframe, sizeof(*frame));
      } else {
          /*

          * For bug-compatibility with older kernels, some space

arch/arm/kernel/signal.c +4 -2
@@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
           * Set the magic and size appropriately so that properly
           * written userspace can skip it reliably:
           */
-        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
-        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
+        *kframe = (struct iwmmxt_sigframe) {
+            .magic = DUMMY_MAGIC,
+            .size  = IWMMXT_STORAGE_SIZE,
+        };

The storage member of kframe is uninitialized, it seems will lead a info leak to userspace ?

In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it has no specific behavior
to define the uninitialized member.

Quoted 7 lines
Quoted 2 lines

Please correct me if I am wrong.

My understanding is that when using a compound initializer (either at variable declaration or by assigning a compound literal like in this case), the unspecified members get initialized to 0.

Also, to back that claim a bit more, when using designated initializers[1]:

"Omitted fields are implicitly initialized the same as for objects that have static storage duration."

[1]
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Inits

Signature · 6 lines
Julien Thierry

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#3 lore raw
Quoted 50 lines · Julien Thierry
Quoted 39 lines · Julien Thierry

Hi Yang,

Quoted 36 lines · Yang Yingliang

Hi, Julien

In this commit 73839798af7e ("ARM: 8790/1: signal: always use __copy_to_user to save iwmmxt context"):

arch/arm/kernel/signal.c +0 -2
@@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
          kframe->magic = IWMMXT_MAGIC;
          kframe->size = IWMMXT_STORAGE_SIZE;
          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
-
-        err = __copy_to_user(frame, kframe, sizeof(*frame));
      } else {
          /*

          * For bug-compatibility with older kernels, some space

arch/arm/kernel/signal.c +4 -2
@@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
           * Set the magic and size appropriately so that properly
           * written userspace can skip it reliably:
           */
-        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
-        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
+        *kframe = (struct iwmmxt_sigframe) {
+            .magic = DUMMY_MAGIC,
+            .size  = IWMMXT_STORAGE_SIZE,
+        };

The storage member of kframe is uninitialized, it seems will lead a info leak to userspace ?

In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it has no specific behavior
to define the uninitialized member.

Quoted 7 lines
Quoted 2 lines

Please correct me if I am wrong.

My understanding is that when using a compound initializer (either at variable declaration or by assigning a compound literal like in this case), the unspecified members get initialized to 0.

Quoted 4 lines

Also, to back that claim a bit more, when using designated initializers[1]:

"Omitted fields are implicitly initialized the same as for objects that have static storage duration."

We also rely on this elsewhere IIUC.

I don't think this guarantee extends to padding though, so watch out for that.

For this case, it looks like struct iwmmxt_sigframe is padding-free though.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#4 lore raw

Hi Julien, Dave,

Quoted 56 lines · Dave Martin
Quoted 50 lines · Julien Thierry
Quoted 39 lines · Julien Thierry

Hi Yang,

Quoted 36 lines · Yang Yingliang

Hi, Julien

In this commit 73839798af7e ("ARM: 8790/1: signal: always use __copy_to_user to save iwmmxt context"):

arch/arm/kernel/signal.c +0 -2
@@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
          kframe->magic = IWMMXT_MAGIC;
          kframe->size = IWMMXT_STORAGE_SIZE;
          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
-
-        err = __copy_to_user(frame, kframe, sizeof(*frame));
      } else {
          /*

          * For bug-compatibility with older kernels, some space

arch/arm/kernel/signal.c +4 -2
@@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
 iwmmxt_sigframe __user *frame)
           * Set the magic and size appropriately so that properly
           * written userspace can skip it reliably:
           */
-        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
-        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
+        *kframe = (struct iwmmxt_sigframe) {
+            .magic = DUMMY_MAGIC,
+            .size  = IWMMXT_STORAGE_SIZE,
+        };

The storage member of kframe is uninitialized, it seems will lead a info leak to userspace ?

In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it has no specific behavior
to define the uninitialized member.

Quoted 7 lines
Quoted 2 lines

Please correct me if I am wrong.

My understanding is that when using a compound initializer (either at variable declaration or by assigning a compound literal like in this case), the unspecified members get initialized to 0.

Quoted 4 lines

Also, to back that claim a bit more, when using designated initializers[1]:

"Omitted fields are implicitly initialized the same as for objects that have static storage duration."

Quoted 7 lines

We also rely on this elsewhere IIUC.

I don't think this guarantee extends to padding though, so watch out for that.

For this case, it looks like struct iwmmxt_sigframe is padding-free though.

Thank you for the clarify, that's crystal clear for us now.

Thanks
Hanjun

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel