Re: [PATCH] selinux: use strlcpy() when copying IB device name

From: Ondrej Mosnacek <omosnace@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH] selinux: use strlcpy() when copying IB device name
Date: Wed, 12 May 2021 15:52:01 +0200	[thread overview]
Message-ID: <CAFqZXNungmX4_x=Q7H0X9xEMznASKXOHJjSVniiFapAHedBKKg@mail.gmail.com> (raw)
In-Reply-To: <CAHC9VhRFT=CVTq=wCxE=Wn8VHP+g1Cyw4zc0oAtsk7Jaa5EMzQ@mail.gmail.com>

On Tue, May 11, 2021 at 3:56 AM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, May 7, 2021 at 9:04 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > While the buffer should be large enough (IB_DEVICE_NAME_MAX) for all
> > InfiniBand device names, it's better to be defensive and ensure the
> > string will be null-terminated even if the hook happens to receive a
> > longer name.
> >
> > Found by a Coverity scan (BUFFER_SIZE warning).
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/hooks.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 92f909a2e8f7..ec14ed56f508 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6864,7 +6864,7 @@ static int selinux_ib_endport_manage_subnet(void *ib_sec, const char *dev_name,
> >                 return err;
> >
> >         ad.type = LSM_AUDIT_DATA_IBENDPORT;
> > -       strncpy(ibendport.dev_name, dev_name, sizeof(ibendport.dev_name));
> > +       strlcpy(ibendport.dev_name, dev_name, sizeof(ibendport.dev_name));
>
> The kernel preference these days appears to be to use strscpy()
> instead of strlcpy(); if we are going to change it, let's change it to
> strscpy().

Good point. But now that you made me look at it again, I noticed that
we can simply turn the dev_name field to a const char * and avoid the
copy altogether. The ibendport variable goes out of scope at the end
of the function anyway, so the lifetime of the dev_name pointer will
never be shorter than that of ibendport, thus we can safely just pass
the dev_name pointer and be done with it.

So I'll update the patch to just switch to this other approach.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.