The following c language code can trigger KASAN's global variable
out-of-bounds access error in kobject_action_type():
int main() {
int fd;
char *filename = "/sys/block/ram12/uevent";
char str[86] = "offline";
int len = 86;
fd = open(filename, O_WRONLY);
if (fd == -1) {
printf("open");
exit(1);
}
if (write(fd, str, len) == -1) {
printf("write");
exit(1);
}
close(fd);
return 0;
}
Function kobject_action_type() receives the input parameters buf and count,
where count is the length of the string buf.
In the use case we provided, count is 86, the count_first is 85.
Buf points to a string with a length of 86, and its first seven characters
are "offline". In the for loop, kobject_actions[action] is the string
"offline" with the length of 7,an out-of-boundary access will appear:
kobject_actions[action][85].
Use sysfs_match_string() to replace the fragile and convoluted loop.
This function is well-tested for parsing sysfs inputs. Moreover, this
modification will not cause any functional changes.
Fixes: f36776fafbaa ("kobject: support passing in variables for synthetic uevents")
Signed-off-by: Xia Fukun <[email protected]>
---
v6 -> v7:
- Move macro UEVENT_KACT_STRSIZE to the .c file to
improve maintainability.
v5 -> v6:
- Ensure that the following extensions remain effective:
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-uevent
v4 -> v5:
- Fixed build errors and warnings, and retested the patch.
v3 -> v4:
- Refactor the function to be more obviously correct and readable.
---
lib/kobject_uevent.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 7c44b7ae4c5c..2171e1648dad 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -47,6 +47,14 @@ static LIST_HEAD(uevent_sock_list);
/* This lock protects uevent_seqnum and uevent_sock_list */
static DEFINE_MUTEX(uevent_sock_mutex);
+/*
+ * The maximum length of the string contained in kobject_actions[].
+ * If there are any actions added or modified, please ensure that
+ * the string length does not exceed the macro, otherwise
+ * should modify the macro definition.
+ */
+#define UEVENT_KACT_STRSIZE 16
+
/* the strings here must match the enum in include/linux/kobject.h */
static const char *kobject_actions[] = {
[KOBJ_ADD] = "add",
@@ -66,7 +74,8 @@ static int kobject_action_type(const char *buf, size_t count,
enum kobject_action action;
size_t count_first;
const char *args_start;
- int ret = -EINVAL;
+ int i, ret = -EINVAL;
+ char kobj_act_buf[UEVENT_KACT_STRSIZE] = {0};
if (count && (buf[count-1] == '\n' || buf[count-1] == '\0'))
count--;
@@ -77,21 +86,24 @@ static int kobject_action_type(const char *buf, size_t count,
args_start = strnchr(buf, count, ' ');
if (args_start) {
count_first = args_start - buf;
+ if (count_first > UEVENT_KACT_STRSIZE)
+ goto out;
+
args_start = args_start + 1;
+ strncpy(kobj_act_buf, buf, count_first);
+ i = sysfs_match_string(kobject_actions, kobj_act_buf);
} else
- count_first = count;
+ i = sysfs_match_string(kobject_actions, buf);
- for (action = 0; action < ARRAY_SIZE(kobject_actions); action++) {
- if (strncmp(kobject_actions[action], buf, count_first) != 0)
- continue;
- if (kobject_actions[action][count_first] != '\0')
- continue;
- if (args)
- *args = args_start;
- *type = action;
- ret = 0;
- break;
- }
+ if (i < 0)
+ goto out;
+
+ action = i;
+ if (args)
+ *args = args_start;
+
+ *type = action;
+ ret = 0;
out:
return ret;
}
--
2.17.1
On 2023/5/18 17:16, Xia Fukun wrote:
> ---
> v6 -> v7:
> - Move macro UEVENT_KACT_STRSIZE to the .c file to
> improve maintainability.
>
Gentle ping ...
UEVENT_KACT_STRSIZE is defined as the maximum length of the string
contained in kobject_actions[].
At present, the maximum length of strings in this array is 7. Based on
the actual meaning of these strings, these actions will not exceed 16
if there are any subsequent changes.
I have submitted v7 of the patch according to your suggestion and
tested it to ensure its functionality is correct.
Please take the time to review it.
Thank you very much.
> ---
> lib/kobject_uevent.c | 38 +++++++++++++++++++++++++-------------
> 1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 7c44b7ae4c5c..2171e1648dad 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -47,6 +47,14 @@ static LIST_HEAD(uevent_sock_list);
> /* This lock protects uevent_seqnum and uevent_sock_list */
> static DEFINE_MUTEX(uevent_sock_mutex);
>
> +/*
> + * The maximum length of the string contained in kobject_actions[].
> + * If there are any actions added or modified, please ensure that
> + * the string length does not exceed the macro, otherwise
> + * should modify the macro definition.
> + */
> +#define UEVENT_KACT_STRSIZE 16
> +
> /* the strings here must match the enum in include/linux/kobject.h */
> static const char *kobject_actions[] = {
> [KOBJ_ADD] = "add",
> @@ -66,7 +74,8 @@ static int kobject_action_type(const char *buf, size_t count,
> enum kobject_action action;
> size_t count_first;
> const char *args_start;
> - int ret = -EINVAL;
> + int i, ret = -EINVAL;
> + char kobj_act_buf[UEVENT_KACT_STRSIZE] = {0};
>
> if (count && (buf[count-1] == '\n' || buf[count-1] == '\0'))
> count--;
> @@ -77,21 +86,24 @@ static int kobject_action_type(const char *buf, size_t count,
> args_start = strnchr(buf, count, ' ');
> if (args_start) {
> count_first = args_start - buf;
> + if (count_first > UEVENT_KACT_STRSIZE)
> + goto out;
> +
> args_start = args_start + 1;
> + strncpy(kobj_act_buf, buf, count_first);
> + i = sysfs_match_string(kobject_actions, kobj_act_buf);
> } else
> - count_first = count;
> + i = sysfs_match_string(kobject_actions, buf);
>
> - for (action = 0; action < ARRAY_SIZE(kobject_actions); action++) {
> - if (strncmp(kobject_actions[action], buf, count_first) != 0)
> - continue;
> - if (kobject_actions[action][count_first] != '\0')
> - continue;
> - if (args)
> - *args = args_start;
> - *type = action;
> - ret = 0;
> - break;
> - }
> + if (i < 0)
> + goto out;
> +
> + action = i;
> + if (args)
> + *args = args_start;
> +
> + *type = action;
> + ret = 0;
> out:
> return ret;
> }
On Wed, Jun 14, 2023 at 07:32:38PM +0800, Xia Fukun wrote:
>
> On 2023/5/18 17:16, Xia Fukun wrote:
> > ---
> > v6 -> v7:
> > - Move macro UEVENT_KACT_STRSIZE to the .c file to
> > improve maintainability.
> >
>
> Gentle ping ...
>
> UEVENT_KACT_STRSIZE is defined as the maximum length of the string
> contained in kobject_actions[].
>
> At present, the maximum length of strings in this array is 7. Based on
> the actual meaning of these strings, these actions will not exceed 16
> if there are any subsequent changes.
>
> I have submitted v7 of the patch according to your suggestion and
> tested it to ensure its functionality is correct.
It's in my to-review queue, but I was hoping that others would at least
test it out given that the previous versions had so many problems. I am
loath to do that on my own, sorry.
> Please take the time to review it.
How did you test it? How have you verified that the previous failures
were caught this time?
You can understand my hesitancy here, right?
thanks,
greg k-h
On 2023/6/15 4:09, Greg KH wrote:
>
> How did you test it? How have you verified that the previous failures
> were caught this time?
>
My testing method is to apply the patch, compile the kernel image,
and start the QEMU virtual machine. Then compile and execute the code
mentioned in the patch that triggers out-of-bounds issues.
In addition, the following operations will be performed to verify the
functions mentioned by Peter Rajnoha <[email protected]>:
# echo "add fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed A=1 B=abc" >
/sys/block/ram0/uevent
# udevadm monitor --kernel --env
monitor will print the received events for:
KERNEL - the kernel uevent
KERNEL[189.376386] add /devices/virtual/block/ram0 (block)
ACTION=add
DEVPATH=/devices/virtual/block/ram0
SUBSYSTEM=block
SYNTH_UUID=fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed
SYNTH_ARG_A=1
SYNTH_ARG_B=abc
DEVNAME=/dev/ram0
DEVTYPE=disk
DISKSEQ=14
SEQNUM=3781
MAJOR=1
MINOR=0
> You can understand my hesitancy here, right?
>
> thanks,
>
> greg k-h
I am not sure which experts can help review this patch, as I can
only find a limited number of people through the git blame command.
On Thu, Jun 15, 2023 at 09:43:34AM +0800, Xia Fukun wrote:
>
> On 2023/6/15 4:09, Greg KH wrote:
>
> >
> > How did you test it? How have you verified that the previous failures
> > were caught this time?
> >
>
> My testing method is to apply the patch, compile the kernel image,
> and start the QEMU virtual machine. Then compile and execute the code
> mentioned in the patch that triggers out-of-bounds issues.
>
> In addition, the following operations will be performed to verify the
> functions mentioned by Peter Rajnoha <[email protected]>:
>
> # echo "add fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed A=1 B=abc" >
> /sys/block/ram0/uevent
>
> # udevadm monitor --kernel --env
> monitor will print the received events for:
> KERNEL - the kernel uevent
>
> KERNEL[189.376386] add /devices/virtual/block/ram0 (block)
> ACTION=add
> DEVPATH=/devices/virtual/block/ram0
> SUBSYSTEM=block
> SYNTH_UUID=fe4d7c9d-b8c6-4a70-9ef1-3d8a58d18eed
> SYNTH_ARG_A=1
> SYNTH_ARG_B=abc
> DEVNAME=/dev/ram0
> DEVTYPE=disk
> DISKSEQ=14
> SEQNUM=3781
> MAJOR=1
> MINOR=0
So you have not run this on any real systems? Please try doing that
(boot your laptop, your server, your server farm, etc.) with it and do a
lot of testing that way.
To only use qemu means it has not really been tested at all, which now
explains why you didn't find the previous problems with this patch
series as this is a kernel path that is HIGHLY dependent on the hardware
involved (i.e. it wants real hardware, not emulated ones.)
thanks,
greg k-h
On Thu, May 18, 2023 at 05:16:14PM +0800, Xia Fukun wrote:
> The following c language code can trigger KASAN's global variable
> out-of-bounds access error in kobject_action_type():
>
> int main() {
> int fd;
> char *filename = "/sys/block/ram12/uevent";
> char str[86] = "offline";
> int len = 86;
>
> fd = open(filename, O_WRONLY);
> if (fd == -1) {
> printf("open");
> exit(1);
> }
>
> if (write(fd, str, len) == -1) {
> printf("write");
> exit(1);
> }
>
> close(fd);
> return 0;
> }
>
> Function kobject_action_type() receives the input parameters buf and count,
> where count is the length of the string buf.
>
> In the use case we provided, count is 86, the count_first is 85.
> Buf points to a string with a length of 86, and its first seven characters
> are "offline". In the for loop, kobject_actions[action] is the string
> "offline" with the length of 7,an out-of-boundary access will appear:
>
> kobject_actions[action][85].
>
> Use sysfs_match_string() to replace the fragile and convoluted loop.
> This function is well-tested for parsing sysfs inputs. Moreover, this
> modification will not cause any functional changes.
>
> Fixes: f36776fafbaa ("kobject: support passing in variables for synthetic uevents")
> Signed-off-by: Xia Fukun <[email protected]>
> ---
> v6 -> v7:
> - Move macro UEVENT_KACT_STRSIZE to the .c file to
> improve maintainability.
>
> v5 -> v6:
> - Ensure that the following extensions remain effective:
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-uevent
>
> v4 -> v5:
> - Fixed build errors and warnings, and retested the patch.
>
> v3 -> v4:
> - Refactor the function to be more obviously correct and readable.
> ---
> lib/kobject_uevent.c | 38 +++++++++++++++++++++++++-------------
> 1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 7c44b7ae4c5c..2171e1648dad 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -47,6 +47,14 @@ static LIST_HEAD(uevent_sock_list);
> /* This lock protects uevent_seqnum and uevent_sock_list */
> static DEFINE_MUTEX(uevent_sock_mutex);
>
> +/*
> + * The maximum length of the string contained in kobject_actions[].
> + * If there are any actions added or modified, please ensure that
> + * the string length does not exceed the macro, otherwise
> + * should modify the macro definition.
> + */
> +#define UEVENT_KACT_STRSIZE 16
But the biggest size here is not 16, it's 6. So where did 16 come from?
Why not dynamically determine the biggest size at runtime?
thanks,
greg k-h