ixl: prevent non-privileged access to NVM update interface

[jacob.e.keller_intel.com]

Summary

Add a privilege check to the ixl_handle_nvmupd_cmd function, ensuring
that only privileged users are allowed to access the NVM update
interface.

Signed-off-by: Jacob Keller <[e-mail]>

[jacob.e.keller_intel.com]

I put the check directly in ixl_handle_nvmupd_cmd, similar to the check that we have in the ice driver code.

[markj]

IMO it makes slightly more sense to have the check in the ixl ioctl handler: it serves as a signal to anyone extending the ioctl handler that they should be careful, and doing so maintains the invariant that internal driver functions need not check for privileges. I am not the maintainer though so I don't insist on any particular approach.

[erj]

In D22870#500744, @markj wrote:

IMO it makes slightly more sense to have the check in the ixl ioctl handler: it serves as a signal to anyone extending the ioctl handler that they should be careful, and doing so maintains the invariant that internal driver functions need not check for privileges. I am not the maintainer though so I don't insist on any particular approach.

Did you mean "iflib" in your comment?

No one's really stepped up to be the official maintainer for iflib, and I'd rather have that privilege check be part of iflib instead of being the job of a driver writer to remember to add. OTOH, that ioctl could be used for reading non-privileged data, like temperature information, so having that privilege check could prevent that ioctl from being used for something that isn't a security risk.

[markj]

In D22870#500774, @erj wrote:

In D22870#500744, @markj wrote:

IMO it makes slightly more sense to have the check in the ixl ioctl handler: it serves as a signal to anyone extending the ioctl handler that they should be careful, and doing so maintains the invariant that internal driver functions need not check for privileges. I am not the maintainer though so I don't insist on any particular approach.

Did you mean "iflib" in your comment?

No one's really stepped up to be the official maintainer for iflib, and I'd rather have that privilege check be part of iflib instead of being the job of a driver writer to remember to add. OTOH, that ioctl could be used for reading non-privileged data, like temperature information, so having that privilege check could prevent that ioctl from being used for something that isn't a security risk.

I mean ixl_if_priv_ioctl(). I'm not sure what you mean by "that" ioctl, since ixl_if_priv_ioctl() handles multiple ioctls by virtue of not looking at the command argument. In particular, iflib passes driver-specific ioctls to ixl_if_priv_ioctl() and so would not seem to be the right place to check for privileges for exactly the reason that you state.

[jacob.e.keller_intel.com]

I mean ixl_if_priv_ioctl(). I'm not sure what you mean by "that" ioctl, since ixl_if_priv_ioctl() handles multiple ioctls by virtue of not looking at the command argument. In particular, iflib passes driver-specific ioctls to ixl_if_priv_ioctl() and so would not seem to be the right place to check for privileges for exactly the reason that you state.

The reason I didn't place the check in ixl_if_priv_ioctl is in the case where we extend with an additional driver specific ioctl that isn't privileged. The reason that this one should be privileged is that it lets you write to chip flash contents. Basically, for the reason Eric stated above. I don't believe we can put the privilege check into the iflib call.

I do think that it should be made more visible that the ioctl is not privilege checked ahead of time, but I'm not really sure what the best way to message that is.

[markj]

In D22870#500864, @jacob.e.keller_intel.com wrote:

I mean ixl_if_priv_ioctl(). I'm not sure what you mean by "that" ioctl, since ixl_if_priv_ioctl() handles multiple ioctls by virtue of not looking at the command argument. In particular, iflib passes driver-specific ioctls to ixl_if_priv_ioctl() and so would not seem to be the right place to check for privileges for exactly the reason that you state.

The reason I didn't place the check in ixl_if_priv_ioctl is in the case where we extend with an additional driver specific ioctl that isn't privileged. The reason that this one should be privileged is that it lets you write to chip flash contents. Basically, for the reason Eric stated above. I don't believe we can put the privilege check into the iflib call.

I do think that it should be made more visible that the ioctl is not privilege checked ahead of time, but I'm not really sure what the best way to message that is.

I am saying that ixl_if_priv_ioctl() should look like this:

static int                                                                                                                                                    
ixl_if_priv_ioctl(if_ctx_t ctx, u_long command, caddr_t data)                                                                                                 
{                                                                                                                                                             
        struct ixl_pf *pf = iflib_get_softc(ctx);                                                                                                             
        struct ifdrv *ifd;                                                                                                                                    
        int error = 0;                                                                                                                                        
                                                                                                                                                              
        switch (command) {                                                                                                                                    
        case SIOCGDRVSPEC:                                                                                                                                  
                ifd = (struct ifdrv *)data;                                                                                                                   
                if (ifd->ifd_cmd == I40E_NVM_ACCESS) {                                                                                                        
                        error = priv_check(curthread, PRIV_DRIVER);                                                                                           
                        if (error != 0)                                                                                                                       
                                break;                                                                                                                        
                        error = ixl_handle_nvmupd_cmd(pf, ifd);                                                                                               
                } else {                                                                                                                                      
                        error = EINVAL;                                                                                                                       
                }                                                                                                                                             
                break;                                                                                                                                        
        default:                                                                                                                                              
                error = EOPNOTSUPP;                                                                                                                           
        }                                                                                                                                                     
                                                                                                                                                              
        return (error);                                                                                                                                       
}

Then you can extend it however you like, and it is clearer to anyone doing so that they may need to add a privilege check.

It also provides future-proofing against new ioctl commands that may be passed through by iflib. Right now ixl_if_priv_ioctl() may be called with one of three ioctls: SIOCGDRVSPEC, SIOCSDRVSPEC and SIOCGPRIVATE_0, but it only handles the first one properly.

[jacob.e.keller_intel.com]

Refactor check as suggested by review comments

[jacob.e.keller_intel.com]

Add missing break statement

[jacob.e.keller_intel.com]

Fix typo of local variable name