On Wed, May 10, 2023 at 06:15:41PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Gently ping.
>
> Is there any interest?
The question that I would've loved to have an answer to was why was it
made O_RDWR and not just O_WRONLY in the first place. Was there a time
when this was meaningful? Because honestly this looks innocent and
straightforward and then it always makes me go and think "Oh, there's
probably a good reason and something super obvious I'm missing.".
Funny enough, this code was originally:
if (open_namei("core",O_CREAT | O_WRONLY | O_TRUNC,0600,&inode,NULL)
and then became:
if (open_namei("core",O_CREAT | 2 | O_TRUNC,0600,&inode,NULL))
in
commit 9cb9f18b5d26 ("[PATCH] Linux-0.99.10 (June 7, 1993)")
Author/applier of said patch Cced (more for the fun of referencing
Linux-0.99.10 than anything else).
So after this commit the flag combination just got copied over and over.
First when coredump handling was moved out of fs/exec.c into the
individual binfmt handlers and then again when it was moved back into
fs/exec.c and then again when it was moved to fs/coredump.c.
So that open-coded 2 added in commit 9cb9f18b5d26 ("[PATCH]
Linux-0.99.10 (June 7, 1993)") survived for 23 years until it was
replaced by Jan in 378c6520e7d2 ("fs/coredump: prevent fsuid=0 dumps
into user-controlled directories").
So no one could be bothered for 23 years to use O_RDWR instead of that
lonely 2 which is kinda funny. :)
In any case, I don't see anything that suggests this would cause issues.
So I'm going to pick this up unless I'm being told I'm being obviously
stupid and this absolutely needs to be O_RDWR...
>
> On 20.04.23 15:04, Vladimir Sementsov-Ogievskiy wrote:
> > This makes it possible to make stricter apparmor profile and don't
> > allow the program to read any coredump in the system.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxxx>
> > ---
> > fs/coredump.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 5df1e6e1eb2b..8f263a389175 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -646,7 +646,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> > } else {
> > struct mnt_idmap *idmap;
> > struct inode *inode;
> > - int open_flags = O_CREAT | O_RDWR | O_NOFOLLOW |
> > + int open_flags = O_CREAT | O_WRONLY | O_NOFOLLOW |
> > O_LARGEFILE | O_EXCL;
> > if (cprm.limit < binfmt->min_coredump)
>
> --
> Best regards,
> Vladimir
>
