| Message ID | b9b0c0c8-6ece-466a-99e4-d49797f69957@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: v4l2-subdev: Fix a 64bit bug | expand |
On 03/11/2023 09:39, Dan Carpenter wrote: > The problem is this line here from subdev_do_ioctl(). > > client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; > > The "client_cap->capabilities" variable is a u64. The AND operation > is supposed to clear out the V4L2_SUBDEV_CLIENT_CAP_STREAMS flag. But > because it's a 32 bit variable it accidentally clears out the high 32 > bits as well. > > Currently we only use the first bit and none of the upper bits so this > doesn't affect runtime behavior. > > Fixes: f57fa2959244 ("media: v4l2-subdev: Add new ioctl for client capabilities") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: Don't use BIT_ULL() because this is UAPI. Also fix a couple typos > in the commit message. > > include/uapi/linux/v4l2-subdev.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 4a195b68f28f..b383c2fe0cf3 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { > * set (which is the default), the 'stream' fields will be forced to 0 by the > * kernel. > */ > - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1ULL << 0) > > /** > * struct v4l2_subdev_client_capability - Capabilities of the client accessing Good catch, thanks! One would wish that the compiler would give a warning on cases like this... How did you find this? Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi
On Fri, Nov 03, 2023 at 09:46:11AM +0200, Tomi Valkeinen wrote: > On 03/11/2023 09:39, Dan Carpenter wrote: > > - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > > + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1ULL << 0) > > /** > > * struct v4l2_subdev_client_capability - Capabilities of the client accessing > > Good catch, thanks! One would wish that the compiler would give a warning on > cases like this... How did you find this? This was an unpublished Smatch warning. I have a comment in my code that I didn't publish this because Sparse already had a warning for this. But that seems not to be true. I'll clean that up a bit and publish it. Thanks! regards, dan carpenter
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index 4a195b68f28f..b383c2fe0cf3 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { * set (which is the default), the 'stream' fields will be forced to 0 by the * kernel. */ - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1ULL << 0) /** * struct v4l2_subdev_client_capability - Capabilities of the client accessing
The problem is this line here from subdev_do_ioctl(). client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; The "client_cap->capabilities" variable is a u64. The AND operation is supposed to clear out the V4L2_SUBDEV_CLIENT_CAP_STREAMS flag. But because it's a 32 bit variable it accidentally clears out the high 32 bits as well. Currently we only use the first bit and none of the upper bits so this doesn't affect runtime behavior. Fixes: f57fa2959244 ("media: v4l2-subdev: Add new ioctl for client capabilities") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: Don't use BIT_ULL() because this is UAPI. Also fix a couple typos in the commit message. include/uapi/linux/v4l2-subdev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)