On 02/02/2023 17:50, Luca Vizzarro wrote:
The function br_dev_read_uargs copies a list of arguments from userspace. Normally, they may contain pointers or real numbers. When working with capabilities, the type unsigned long used for this purpose is not big enough to hold them. This commit changes the type of args from unsigned long to user_uintptr_t for this purpose. As a consequence the type of the variables fed to this function is also updated, and casted to unsigned long when treated as a number. It also fixes the type of argp, as it points to an entry of args, which lives in the kernel space, and not in the user space as the type suggests.
That is a good catch! I'm really surprised Clang accepted this without warning... This fixup was introduced by a previous commit, so you could add:
Fixes: ("net: Fix the position of __capability in double pointer")
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
net/bridge/br_ioctl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c index 9c3030de438c..aa4e34cf8754 100644 --- a/net/bridge/br_ioctl.c +++ b/net/bridge/br_ioctl.c @@ -104,9 +104,9 @@ static int add_del_if(struct net_bridge *br, int ifindex, int isadd) } #define BR_UARGS_MAX 4 -static int br_dev_read_uargs(unsigned long *args, size_t nr_args, +static int br_dev_read_uargs(user_uintptr_t *args, size_t nr_args,
This made me pause as it's a change in ABI. All things considered I thing it makes sense, that's the most coherent way to approach it. Fortunately it should impact very little software, especially since this is only used for old APIs and most software only uses them as fallback. Still, I found at least one case (toybox [1]) where only the old APIs are used, so this may not go entirely unnoticed (unsigned long will need to be replaced with uintptr_t for the array and casts). I'll add this to the PCuABI spec.
On a more practical note, you need an additional change if you start reading user pointers from memory this way: the copy_from_user() below needs to become copy_from_user_with_ptr() to preserve capability tags.
[1] https://github.com/landley/toybox/blob/master/toys/pending/brctl.c
#ifdef CONFIG_CHERI_PURECAP_UABI
void * __capability * __capability argp, void __user *data)
void * __capability *argp, void __user *data)#else void __user **argp, void __user *data) #endif @@ -150,7 +150,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, { struct net_bridge *br = netdev_priv(dev); struct net_bridge_port *p = NULL;
- unsigned long args[4];
- user_uintptr_t args[4]; void __user *argp; int ret;
@@ -342,7 +342,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, static int old_deviceless(struct net *net, void __user *data) {
- unsigned long args[3];
- user_uintptr_t args[3]; void __user *argp; int ret;
@@ -369,7 +369,7 @@ static int old_deviceless(struct net *net, void __user *data) ret = copy_to_user(argp, indices, array_size(args[2], sizeof(int)))
? -EFAULT : args[2];
? -EFAULT : (unsigned long)args[2];
int surely? Both sides of the ternary should have the same type (and ret is an int), I'm surprised the compiler doesn't complain about that.
Kevin
kfree(indices); return ret;