Messages in this thread | | | Date | Thu, 31 Mar 2022 22:35:37 +0200 | Subject | Re: [PATCH] binderfs: Fix the maximum minor value in binderfs_binder_device_create() and binderfs_binder_ctl_create() | From | Christophe JAILLET <> |
| |
Le 29/03/2022 à 13:20, Christian Brauner a écrit : > On Sun, Mar 27, 2022 at 01:18:17PM +0200, Christophe JAILLET wrote: >> ida_alloc_max(..., max, ...) returns values from 0 to max, inclusive. >> >> So, BINDERFS_MAX_MINOR is a valid value for 'minor'. >> >> BINDERFS_MAX_MINOR is '1U << MINORBITS' and we have: >> #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi)) >> >> So, When this value is used in MKDEV() and it will overflow. >> >> Fixes: 3ad20fe393b3 ("binder: implement binderfs") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> This patch is completely speculative. >> >> The 'BINDERFS_MAX_MINOR_CAPPED - 1' is here only for symmetry with the >> BINDERFS_MAX_MINOR case. I'm not sure at all that is is needed and, more >> importantly, that it is correct. > > Hm, since we're "removing" one alloctable device for the initial ipc > namespace, I think we need the -1 for the capped value. > > Though I wonder if the simpler fix wouldn't just be to: > > #define BINDERFS_MAX_MINOR MINORMASK > > since include/linux/kdev_t.h has: > > #define MINORBITS 20 > #define MINORMASK ((1U << MINORBITS) - 1) >
Hi, I mostly agree with you, but I don't have a strong opinion on the other uses of BINDERFS_MAX_MINOR.
The ones related to 'max' values looks good to me, but I don't know the implication for the one used in binderfs_make_inode() and in init_binderfs().
I won't be able to help further here.
CJ
| |