linux-kernel - Re: [PATCH] comedi: zero-init data in do_insn

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]

Message-ID: <7cc194c0-1ce2-411b-92e3-fb1b6c63f062@mev.co.uk>
Date: Fri, 25 Jul 2025 10:41:27 +0100
From: Ian Abbott <abbotti@....co.uk>
To: Arnaud Lecomte <contact@...aud-lcm.com>, hsweeten@...ionengravers.com,
 gregkh@...uxfoundation.org,
 syzbot+a5e45f768aab5892da5d@...kaller.appspotmail.com
Cc: jannh@...gle.com, linux-kernel@...r.kernel.org,
 syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] comedi: zero-init data in do_insn_ioctl

On 24/07/2025 22:04, Arnaud Lecomte wrote:
> KMSAN reported a kernel-infoleak when copying instruction data back to
> userspace in do_insnlist_ioctl(). The issue occurs because allocated
> memory buffers weren't properly initialized (not
> zero initialized)  before being copied to
> userspace, potentially leaking kernel memory.
> 
> Reported-by: syzbot+a5e45f768aab5892da5d@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=a5e45f768aab5892da5d
> Tested-by: syzbot+a5e45f768aab5892da5d@...kaller.appspotmail.com
> Signed-off-by: Arnaud Lecomte <contact@...aud-lcm.com>
> ---
>   drivers/comedi/comedi_fops.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
> index c83fd14dd7ad..15fee829d14c 100644
> --- a/drivers/comedi/comedi_fops.c
> +++ b/drivers/comedi/comedi_fops.c
> @@ -1636,7 +1636,7 @@ static int do_insn_ioctl(struct comedi_device *dev,
>   		n_data = MAX_SAMPLES;
>   	}
>   
> -	data = kmalloc_array(n_data, sizeof(unsigned int), GFP_KERNEL);
> +	data = kcalloc(n_data, sizeof(unsigned int), GFP_KERNEL);
>   	if (!data) {
>   		ret = -ENOMEM;
>   		goto error;

I thought my commit 46d8c744136c ("comedi: Fix initialization of data 
for instructions that write to subdevice" would have fixed this as long 
as all the driver code was playing nicely, but it seems I was mistaken.

I don't think it is necessary to use kcalloc().  The buffer already gets 
initialized (partly by `copy_from_user()`) when `insn->insn & 
INSN_MASK_WRITE` is non-zero, so it just needs to be initialized when 
`insn->insn & INSN_MASK_WRITE` is zero too.

There is nearly identical code in `do_insnlist_ioctl()` that needs 
fixing, so it would be better to fix both at the same time.

-- 
-=( Ian Abbott <abbotti@....co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-