linux-kernel - [PATCH] Using a pointer and kzalloc in place of a struct directly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 12 Sep 2020 17:08:04 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     unlisted-recipients:; (no To-header on input)
Cc:     andriin@...com, ast@...nel.org, bpf@...r.kernel.org,
        daniel@...earbox.net, davem@...emloft.net, hawk@...nel.org,
        john.fastabend@...il.com, kafai@...com, kpsingh@...omium.org,
        kuba@...nel.org, linux-kernel@...r.kernel.org,
        anant.thazhemadam@...il.com
Subject: [PATCH] Using a pointer and kzalloc in place of a struct directly 

Updated the usage of a struct variable directly, in bpf_link_get_info_by_fd
to using a pointer of the same type instead, which points to a memory 
location allocated using kzalloc.

Signed-off-by: Anant Thazhemadam <anant.thazhemadam@...il.com>
---
I saw this bug (https://syzkaller.appspot.com/bug?extid=976d5ecfab0c7eb43ac3),
and tried to come up with a patch for it (before I saw that this had already 
been taken care of). 
Although I don't think it fundamentally changes how things work much, it still 
seems to have fixed the error on it's own too.
I'd like to hear anyone's 2c on this, and know  if this method of using info 
(of type bpf_link_info) instead
would be a welcome change in general, even if it was not centered around 
fixing the bug.
If instead, as an unwelcome consequence, this patch might make something go 
wrong somewhere, or passing
the syzbot test was a false positive, I would appreciate it if you could shed 
some light on that for me as well.
If this patch seems acceptable, then I'll send in a cleaner v2 that's a little
more articulate, if required.
Just trying to understand how things work, and sometimes why things work
in and around the kernel.
Thanks,
Anant


 kernel/bpf/syscall.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4108ef3b828b..01b9c203ef65 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3605,30 +3605,31 @@ static int bpf_link_get_info_by_fd(struct file *file,
 				  union bpf_attr __user *uattr)
 {
 	struct bpf_link_info __user *uinfo = u64_to_user_ptr(attr->info.info);
-	struct bpf_link_info info;
+	struct bpf_link_info *info = NULL;
 	u32 info_len = attr->info.info_len;
 	int err;
 
-	err = bpf_check_uarg_tail_zero(uinfo, sizeof(info), info_len);
+	err = bpf_check_uarg_tail_zero(uinfo, sizeof(struct bpf_link_info), info_len);
+
 	if (err)
 		return err;
 	info_len = min_t(u32, sizeof(info), info_len);
 
-	memset(&info, 0, sizeof(info));
-	if (copy_from_user(&info, uinfo, info_len))
+	info = kzalloc(sizeof(struct bpf_link_info), GFP_KERNEL);
+	if (copy_from_user(info, uinfo, info_len))
 		return -EFAULT;
 
-	info.type = link->type;
-	info.id = link->id;
-	info.prog_id = link->prog->aux->id;
+	info->type = link->type;
+	info->id = link->id;
+	info->prog_id = link->prog->aux->id;
 
 	if (link->ops->fill_link_info) {
-		err = link->ops->fill_link_info(link, &info);
+		err = link->ops->fill_link_info(link, info);
 		if (err)
 			return err;
 	}
 
-	if (copy_to_user(uinfo, &info, info_len) ||
+	if (copy_to_user(uinfo, info, info_len) ||
 	    put_user(info_len, &uattr->info.info_len))
 		return -EFAULT;
 
-- 
2.25.1