linux-kernel - Re: [PATCH] loop: mask loop_control

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: <94A72A7C-0E1A-4C61-ADDE-9BACEA91FDB5@icloud.com>
Date:   Fri, 19 Nov 2021 09:50:14 +0800
From:   wangyangbo <yangbonis@...oud.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     axboe@...nel.dk, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] loop: mask loop_control_ioctl parameter only as minor



> On Nov 18, 2021, at 22:15, Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp> wrote:
> 
> On 2021/11/18 11:36, wangyangbo wrote:
>> @@ -2170,11 +2170,11 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
>> {
>>    switch (cmd) {
>>    case LOOP_CTL_ADD:
>> -        return loop_add(parm);
>> +        return loop_add(MINOR(parm));
> 
> Better to return -EINVAL or something if out of minor range?
Definitely, EINVAL or EDOM, which do you think is better?

> 
>>    case LOOP_CTL_REMOVE:
>> -        return loop_control_remove(parm);
>> +        return loop_control_remove(MINOR(parm));
> 
> This is bad, for this change makes
> 
>    if (idx < 0) {
>        pr_warn("deleting an unspecified loop device is not supported.\n");
>        return -EINVAL;
>    }
> 
> dead code by masking the argument to 0-1048575 range.

But ioctl param is unsigned long, I think this need to sanitize.

>>    case LOOP_CTL_GET_FREE:
>> -        return loop_control_get_free(parm);
>> +        return loop_control_get_free(MINOR(parm));
> 
> This is pointless, for the passed argument is not used.
> By the way, didn't someone already propose removal of this argument?

I don't find this in mail list, but I would like to sanitize that code.