linux-kernel - Re: [RFC PATCH v3 05/10] mm/mshare: Add ioctl support

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: <345a8c5e-1f7d-4d73-a3a0-7d0040e5d5a6@oracle.com>
Date: Tue, 15 Oct 2024 17:49:53 -0700
From: Anthony Yznaga <anthony.yznaga@...cle.com>
To: Jann Horn <jannh@...gle.com>
Cc: akpm@...ux-foundation.org, willy@...radead.org, markhemm@...glemail.com,
        viro@...iv.linux.org.uk, david@...hat.com, khalid@...nel.org,
        andreyknvl@...il.com, dave.hansen@...el.com, luto@...nel.org,
        brauner@...nel.org, arnd@...db.de, ebiederm@...ssion.com,
        catalin.marinas@....com, linux-arch@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org, mhiramat@...nel.org,
        rostedt@...dmis.org, vasily.averin@...ux.dev, xhao@...ux.alibaba.com,
        pcc@...gle.com, neilb@...e.de, maz@...nel.org
Subject: Re: [RFC PATCH v3 05/10] mm/mshare: Add ioctl support


On 10/14/24 1:08 PM, Jann Horn wrote:
> On Wed, Sep 4, 2024 at 1:22 AM Anthony Yznaga <anthony.yznaga@...cle.com> wrote:
>> Reserve a range of ioctls for msharefs and add the first two ioctls
>> to get and set the start address and size of an mshare region.
> [...]
>> +static long
>> +msharefs_set_size(struct mm_struct *mm, struct mshare_data *m_data,
>> +                       struct mshare_info *minfo)
>> +{
>> +       unsigned long end = minfo->start + minfo->size;
>> +
>> +       /*
>> +        * Validate alignment for start address, and size
>> +        */
>> +       if ((minfo->start | end) & (PGDIR_SIZE - 1)) {
>> +               spin_unlock(&m_data->m_lock);
>> +               return -EINVAL;
>> +       }
>> +
>> +       mm->mmap_base = minfo->start;
>> +       mm->task_size = minfo->size;
>> +       if (!mm->task_size)
>> +               mm->task_size--;
>> +
>> +       m_data->minfo.start = mm->mmap_base;
>> +       m_data->minfo.size = mm->task_size;
>> +       spin_unlock(&m_data->m_lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static long
>> +msharefs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> +{
>> +       struct mshare_data *m_data = filp->private_data;
>> +       struct mm_struct *mm = m_data->mm;
>> +       struct mshare_info minfo;
>> +
>> +       switch (cmd) {
>> +       case MSHAREFS_GET_SIZE:
>> +               spin_lock(&m_data->m_lock);
>> +               minfo = m_data->minfo;
>> +               spin_unlock(&m_data->m_lock);
>> +
>> +               if (copy_to_user((void __user *)arg, &minfo, sizeof(minfo)))
>> +                       return -EFAULT;
>> +
>> +               return 0;
>> +
>> +       case MSHAREFS_SET_SIZE:
>> +               if (copy_from_user(&minfo, (struct mshare_info __user *)arg,
>> +                       sizeof(minfo)))
>> +                       return -EFAULT;
>> +
>> +               /*
>> +                * If this mshare region has been set up once already, bail out
>> +                */
>> +               spin_lock(&m_data->m_lock);
>> +               if (m_data->minfo.start != 0) {
> Is there actually anything that prevents msharefs_set_size() from
> setting up m_data with ->minfo.start==0, so that a second
> MSHAREFS_SET_SIZE invocation will succeed? It would probably be more
> reliable to have a separate flag for "has this thing been set up yet".

Thanks for pointing this out. Yes, this is problematic. A start address 
of 0 generally won't work because mmap() will fail unless there are 
sufficient privileges (cap_map_addr will return -EPERM). I already have 
changes to use the size to indicate initialization, but it may make 
sense to have flags.


Anthony

>
>
>> +                       spin_unlock(&m_data->m_lock);
>> +                       return -EINVAL;
>> +               }
>> +
>> +               return msharefs_set_size(mm, m_data, &minfo);
>> +
>> +       default:
>> +               return -ENOTTY;
>> +       }
>> +}