qemu_fw_cfg: Make fw_cfg_rev_attr a proper kobj_attribute

PATCH Applied Nathan Chancellor 6 participants 12 messages Feb 11, 2021 – Apr 2, 2021 Qemu machine emulator and virtualizer support

lore

Share & tools

lore

Nathan Chancellor

author

2021-02-11 19:44 UTC #0 lore raw

PATCH 1/1 qemu_fw_cfg: Make fw_cfg_rev_attr a proper kobj_attribute

fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.

$ cat /sys/firmware/qemu_fw_cfg/rev
3

$ dmesg | grep "CFI failure"
[   26.016832] CFI failure (target: fw_cfg_showrev+0x0/0x8):

Fix this by converting fw_cfg_rev_attr to 'struct kobj_attribute' where this would have been caught automatically by the incompatible pointer types compiler warning. Update fw_cfg_showrev() accordingly.

Fixes 75f3e8e47f38 ("firmware: introduce sysfs driver for QEMU's fw_cfg device")

Link https://github.com/ClangBuiltLinux/linux/issues/1299

Signed-off-by Nathan Chancellor <[email protected]>

---
 drivers/firmware/qemu_fw_cfg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/firmware/qemu_fw_cfg.c +3 -5

index 0078260fbabe..172c751a4f6c 100644
@@ -299,15 +299,13 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static ssize_t fw_cfg_showrev(struct kobject *k, struct attribute *a, char *buf)
+static ssize_t fw_cfg_showrev(struct kobject *k, struct kobj_attribute *a,
+			      char *buf)
 {
 	return sprintf(buf, "%u\n", fw_cfg_rev);
 }
 
-static const struct {
-	struct attribute attr;
-	ssize_t (*show)(struct kobject *k, struct attribute *a, char *buf);
-} fw_cfg_rev_attr = {
+static const struct kobj_attribute fw_cfg_rev_attr = {
 	.attr = { .name = "rev", .mode = S_IRUSR },
 	.show = fw_cfg_showrev,
 };

base-commit: 92bf22614b21a2706f4993b278017e437f7785b3

Signature · 1 line

2.30.1

Sami Tolvanen

Reviewed-by on v1 1/1

2021-02-11 20:42 UTC #1 lore raw

Hi Nathan,

Quoted 13 lines · Nathan Chancellor

fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
$ dmesg | grep "CFI failure"
[   26.016832] CFI failure (target: fw_cfg_showrev+0x0/0x8):

Quoted 7 lines

Fix this by converting fw_cfg_rev_attr to 'struct kobj_attribute' where this would have been caught automatically by the incompatible pointer types compiler warning. Update fw_cfg_showrev() accordingly.
Fixes 75f3e8e47f38 ("firmware: introduce sysfs driver for QEMU's fw_cfg device")
Link https://github.com/ClangBuiltLinux/linux/issues/1299
Signed-off-by Nathan Chancellor <[email protected]>

Looks good to me. Thank you for sending the patch!

Reviewed-by Sami Tolvanen <[email protected]>

Sami

Sedat Dilek

Tested-by

2021-02-24 18:10 UTC #2 lore raw

Quoted 21 lines · Sami Tolvanen' via Clang Built Linux

Hi Nathan,
Quoted 17 lines · Nathan Chancellor
fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
$ dmesg | grep "CFI failure"
[   26.016832] CFI failure (target: fw_cfg_showrev+0x0/0x8):
Fix this by converting fw_cfg_rev_attr to 'struct kobj_attribute' where this would have been caught automatically by the incompatible pointer types compiler warning. Update fw_cfg_showrev() accordingly.

Quoted 8 lines

Quoted 3 lines
Fixes 75f3e8e47f38 ("firmware: introduce sysfs driver for QEMU's fw_cfg device")
Link https://github.com/ClangBuiltLinux/linux/issues/1299
Signed-off-by Nathan Chancellor <[email protected]>
Looks good to me. Thank you for sending the patch!
Reviewed-by Sami Tolvanen <[email protected]>

Environment: Linux v5.11-10201-gc03c21ba6f4e plus Clang-CFI as of 24-Feb-2021 on top built with LLVM v13-git.

Tested-by Sedat Dilek <[email protected]>

- Sedat -

Nick Desaulniers

2021-02-22 19:03 UTC #3 lore raw

In reply to Nathan Chancellor

Did this happen to get picked up already? EOM

Quoted 56 lines · Nathan Chancellor

fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
$ dmesg | grep "CFI failure"
[   26.016832] CFI failure (target: fw_cfg_showrev+0x0/0x8):
Fix this by converting fw_cfg_rev_attr to 'struct kobj_attribute' where this would have been caught automatically by the incompatible pointer types compiler warning. Update fw_cfg_showrev() accordingly.
Fixes 75f3e8e47f38 ("firmware: introduce sysfs driver for QEMU's fw_cfg device")
Link https://github.com/ClangBuiltLinux/linux/issues/1299
Signed-off-by Nathan Chancellor <[email protected]>
---
 drivers/firmware/qemu_fw_cfg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/firmware/qemu_fw_cfg.c +3 -5
index 0078260fbabe..172c751a4f6c 100644
@@ -299,15 +299,13 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
        return 0;
 }
 
-static ssize_t fw_cfg_showrev(struct kobject *k, struct attribute *a, char *buf)
+static ssize_t fw_cfg_showrev(struct kobject *k, struct kobj_attribute *a,
+                             char *buf)
 {
        return sprintf(buf, "%u\n", fw_cfg_rev);
 }
 
-static const struct {
-       struct attribute attr;
-       ssize_t (*show)(struct kobject *k, struct attribute *a, char *buf);
-} fw_cfg_rev_attr = {
+static const struct kobj_attribute fw_cfg_rev_attr = {
        .attr = { .name = "rev", .mode = S_IRUSR },
        .show = fw_cfg_showrev,
 };
base-commit: 92bf22614b21a2706f4993b278017e437f7785b3 --
2.30.1
--
You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210211194258.4137998-1-nathan%40kernel.org.

Signature · 2 lines

Thanks,
~Nick Desaulniers

Nathan Chancellor

author

2021-02-22 19:14 UTC #4 lore raw

Quoted 1 line · Nick Desaulniers

Did this happen to get picked up already? EOM

I have not gotten an email saying it has been picked up nor does it appear to be in -next.

Cheers,
Nathan

Quoted 53 lines · Nathan Chancellor

Quoted 52 lines
fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
$ dmesg | grep "CFI failure"
[   26.016832] CFI failure (target: fw_cfg_showrev+0x0/0x8):
Fix this by converting fw_cfg_rev_attr to 'struct kobj_attribute' where this would have been caught automatically by the incompatible pointer types compiler warning. Update fw_cfg_showrev() accordingly.
Fixes 75f3e8e47f38 ("firmware: introduce sysfs driver for QEMU's fw_cfg device")
Link https://github.com/ClangBuiltLinux/linux/issues/1299
Signed-off-by Nathan Chancellor <[email protected]>
---
 drivers/firmware/qemu_fw_cfg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/firmware/qemu_fw_cfg.c +3 -5
index 0078260fbabe..172c751a4f6c 100644
@@ -299,15 +299,13 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
        return 0;
 }
 
-static ssize_t fw_cfg_showrev(struct kobject *k, struct attribute *a, char *buf)
+static ssize_t fw_cfg_showrev(struct kobject *k, struct kobj_attribute *a,
+                             char *buf)
 {
        return sprintf(buf, "%u\n", fw_cfg_rev);
 }
 
-static const struct {
-       struct attribute attr;
-       ssize_t (*show)(struct kobject *k, struct attribute *a, char *buf);
-} fw_cfg_rev_attr = {
+static const struct kobj_attribute fw_cfg_rev_attr = {
        .attr = { .name = "rev", .mode = S_IRUSR },
        .show = fw_cfg_showrev,
 };
base-commit: 92bf22614b21a2706f4993b278017e437f7785b3 --
2.30.1

Kees Cook

Reviewed-by on v1 1/1

2021-02-25 04:58 UTC #5 lore raw

In reply to Nathan Chancellor

Quoted 12 lines · Nathan Chancellor

fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
$ dmesg | grep "CFI failure"
[   26.016832] CFI failure (target: fw_cfg_showrev+0x0/0x8):

Quoted 7 lines

Fix this by converting fw_cfg_rev_attr to 'struct kobj_attribute' where this would have been caught automatically by the incompatible pointer types compiler warning. Update fw_cfg_showrev() accordingly.
Fixes 75f3e8e47f38 ("firmware: introduce sysfs driver for QEMU's fw_cfg device")
Link https://github.com/ClangBuiltLinux/linux/issues/1299
Signed-off-by Nathan Chancellor <[email protected]>

Ah, nice, yes.

Reviewed-by Kees Cook <[email protected]>

Michael, are you able to take this? I can snag it if needed.

-Kees

Quoted 32 lines

---
 drivers/firmware/qemu_fw_cfg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/firmware/qemu_fw_cfg.c +3 -5

index 0078260fbabe..172c751a4f6c 100644
@@ -299,15 +299,13 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static ssize_t fw_cfg_showrev(struct kobject *k, struct attribute *a, char *buf)
+static ssize_t fw_cfg_showrev(struct kobject *k, struct kobj_attribute *a,
+			      char *buf)
 {
 	return sprintf(buf, "%u\n", fw_cfg_rev);
 }
 
-static const struct {
-	struct attribute attr;
-	ssize_t (*show)(struct kobject *k, struct attribute *a, char *buf);
-} fw_cfg_rev_attr = {
+static const struct kobj_attribute fw_cfg_rev_attr = {
 	.attr = { .name = "rev", .mode = S_IRUSR },
 	.show = fw_cfg_showrev,
 };

base-commit: 92bf22614b21a2706f4993b278017e437f7785b3

Signature · 1 line

2.30.1

Signature · 1 line

Kees Cook

Philippe Mathieu-Daudé

Reviewed-by on v1 1/1

2021-02-25 10:43 UTC #6 lore raw

In reply to Nathan Chancellor

Quoted 16 lines · Nathan Chancellor

fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
$ dmesg | grep "CFI failure"
[   26.016832] CFI failure (target: fw_cfg_showrev+0x0/0x8):
Fix this by converting fw_cfg_rev_attr to 'struct kobj_attribute' where this would have been caught automatically by the incompatible pointer types compiler warning. Update fw_cfg_showrev() accordingly.

Quoted 6 lines

Fixes 75f3e8e47f38 ("firmware: introduce sysfs driver for QEMU's fw_cfg device")
Link https://github.com/ClangBuiltLinux/linux/issues/1299
Signed-off-by Nathan Chancellor <[email protected]>
---
 drivers/firmware/qemu_fw_cfg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Kees Cook

Applied

2021-02-25 21:26 UTC #7 lore raw

In reply to Nathan Chancellor

Quoted 10 lines · Nathan Chancellor

fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
[...]

Applied to kspp/cfi/cleanups, thanks!

[1/1] qemu_fw_cfg: Make fw_cfg_rev_attr a proper kobj_attribute

      https://git.kernel.org/kees/c/f5c4679d6c49

Signature · 1 line

Kees Cook

Sedat Dilek

Tested-by

2021-04-02 06:42 UTC #8 lore raw

Quoted 11 lines · Kees Cook

Quoted 9 lines · Nathan Chancellor
fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3

Quoted 7 lines

Quoted 1 line
[...]
Applied to kspp/cfi/cleanups, thanks!
[1/1] qemu_fw_cfg: Make fw_cfg_rev_attr a proper kobj_attribute
      https://git.kernel.org/kees/c/f5c4679d6c49

I have queued this up in my custom patchset
(for-5.12/kspp-cfi-cleanups-20210225).

What is the plan to get this upstream?

Feel free to add my:

Tested-by Sedat Dilek <[email protected]>

- Sedat -

Kees Cook

2021-04-02 18:25 UTC #9 lore raw

Quoted 16 lines · Sedat Dilek

Quoted 15 lines · Kees Cook
Quoted 10 lines · Nathan Chancellor
fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
[...]
Applied to kspp/cfi/cleanups, thanks!

Quoted 8 lines

Quoted 3 lines
[1/1] qemu_fw_cfg: Make fw_cfg_rev_attr a proper kobj_attribute
      https://git.kernel.org/kees/c/f5c4679d6c49
I have queued this up in my custom patchset
(for-5.12/kspp-cfi-cleanups-20210225).
What is the plan to get this upstream?

I haven't sent it to Linus yet -- I was expecting to batch more of these and send them for v5.13. (But if the kvm folks snag it, that's good too.)

-Kees

Quoted 6 lines

Feel free to add my:
Tested-by Sedat Dilek <[email protected]>
- Sedat -

Signature · 1 line

Kees Cook

Nathan Chancellor

author

2021-04-02 18:31 UTC #10 lore raw

Quoted 21 lines · Kees Cook

Quoted 20 lines · Sedat Dilek
Quoted 18 lines · Kees Cook
Quoted 10 lines · Nathan Chancellor
fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
[...]
Applied to kspp/cfi/cleanups, thanks!
[1/1] qemu_fw_cfg: Make fw_cfg_rev_attr a proper kobj_attribute
      https://git.kernel.org/kees/c/f5c4679d6c49

Quoted 8 lines

Quoted 4 lines
I have queued this up in my custom patchset
(for-5.12/kspp-cfi-cleanups-20210225).
What is the plan to get this upstream?
I haven't sent it to Linus yet -- I was expecting to batch more of these and send them for v5.13. (But if the kvm folks snag it, that's good too.)

I am going to be putting the CFI series through its paces on both arm64 and x86_64 over the next week or so on several different machines (in fact, I am writing up a report right now) so I will probably have some more of these as I find them.

Cheers,
Nathan

Sedat Dilek

2021-04-02 18:45 UTC #11 lore raw

Quoted 28 lines · Nathan Chancellor

Quoted 26 lines · Kees Cook
Quoted 24 lines · Sedat Dilek
Quoted 11 lines · Kees Cook
Quoted 9 lines · Nathan Chancellor
fw_cfg_showrev() is called by an indirect call in kobj_attr_show(), which violates clang's CFI checking because fw_cfg_showrev()'s second parameter is 'struct attribute', whereas the ->show() member of 'struct kobj_structure' expects the second parameter to be of type 'struct kobj_attribute'.
$ cat /sys/firmware/qemu_fw_cfg/rev
3
Quoted 7 lines
Quoted 1 line
[...]
Applied to kspp/cfi/cleanups, thanks!
[1/1] qemu_fw_cfg: Make fw_cfg_rev_attr a proper kobj_attribute
      https://git.kernel.org/kees/c/f5c4679d6c49
I have queued this up in my custom patchset
(for-5.12/kspp-cfi-cleanups-20210225).
What is the plan to get this upstream?

Quoted 9 lines

Quoted 3 lines
I haven't sent it to Linus yet -- I was expecting to batch more of these and send them for v5.13. (But if the kvm folks snag it, that's good too.)
I am going to be putting the CFI series through its paces on both arm64 and x86_64 over the next week or so on several different machines (in fact, I am writing up a report right now) so I will probably have some more of these as I find them.

This was just a friendly ping.

Sami has sent some patches which I reported in the early stage of clang-cfi (x86-64) through subtree maintainers.
It's up to you Nathan or kvm folks.

Upstreamed patches means to me a RDC-ed custom patchset.

- Sedat -