Page MenuHomeFreeBSD

ixl: prevent non-privileged access to NVM update interface
ClosedPublic

Authored by jacob.e.keller_intel.com on Dec 18 2019, 11:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 12:19 AM
Unknown Object (File)
Feb 16 2024, 6:09 AM
Unknown Object (File)
Jan 8 2024, 2:28 AM
Unknown Object (File)
Jan 8 2024, 2:28 AM
Unknown Object (File)
Jan 8 2024, 2:28 AM
Unknown Object (File)
Dec 20 2023, 6:10 AM
Unknown Object (File)
Dec 17 2023, 7:06 AM
Unknown Object (File)
Dec 10 2023, 2:29 PM
Subscribers

Details

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 <jacob.e.keller@intel.com>

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28237
Build 26360: arc lint + arc unit

Event Timeline

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

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.

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.

In D22870#500774, @erj 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.

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

Refactor check as suggested by review comments

This revision is now accepted and ready to land.Dec 20 2019, 6:03 PM