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

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

[Ondrej Mosnacek]

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));
 	ibendport.port = port_num;
 	ad.u.ibendport = &ibendport;
 	return avc_has_perm(&selinux_state,
-- 
2.31.1

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

[Paul Moore]

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().

-- 
paul moore
www.paul-moore.com

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

[Ondrej Mosnacek]

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.