linux-kernel - Re: [PATCH 3/3] fs/file.c: move sanity_check from alloc_fd() to put_unused

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: <CAGudoHHaH8NgdxwC1gr-XdyFSzSfPsD__6jMymTC-FQ7=o_ERw@mail.gmail.com>
Date: Tue, 18 Jun 2024 11:06:49 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Michal Hocko <mhocko@...e.com>
Cc: Tim Chen <tim.c.chen@...ux.intel.com>, Yu Ma <yu.ma@...el.com>, viro@...iv.linux.org.uk, 
	brauner@...nel.org, jack@...e.cz, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, tim.c.chen@...el.com, pan.deng@...el.com, 
	tianyou.li@...el.com
Subject: Re: [PATCH 3/3] fs/file.c: move sanity_check from alloc_fd() to put_unused_fd()

On Tue, Jun 18, 2024 at 10:35 AM Michal Hocko <mhocko@...e.com> wrote:
>
> On Mon 17-06-24 11:04:41, Tim Chen wrote:
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 3a2df1bd9f64..b4e523728c3e 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -1471,6 +1471,7 @@ static int do_prlimit(struct task_struct *tsk, unsigned int resource,
> >                 return -EINVAL;
> >         resource = array_index_nospec(resource, RLIM_NLIMITS);
> >
> > +       task_lock(tsk->group_leader);
> >         if (new_rlim) {
> >                 if (new_rlim->rlim_cur > new_rlim->rlim_max)
> >                         return -EINVAL;
>
> This is clearly broken as it leaves the lock behind on the error, no?

As I explained in my other e-mail there is no need to synchronize
against rlimit changes, merely the code needs to honor the observed
value, whatever it is.

This holds for the stock kernel, does not hold for v1 of the patchset
and presumably will be addressed in v2.

Even if some synchronization was required, moving the lock higher does
not buy anything because the newly protected area only validates the
new limit before there is any attempt at setting it and the old limit
is not looked at.

I think we can safely drop this area and patiently wait for v2.

-- 
Mateusz Guzik <mjguzik gmail.com>