Re: Potential bug in gpiolib-cdev.c in v1 notification about line info changes

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Jun 15, 2021 at 06:57:03PM +0000, Gabriel Knezek wrote:
> Hello GPIO maintainers,
> 
> While upgrading our system from the 5.4 to 5.10 kernel release, we noticed this potential defect in the gpiolib-cdev.c file: https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib-cdev.c#L2255
> 
> In the lineinfo_watch_read routine,
> 
> } else {
>                 struct gpioline_info_changed event_v1;
>                 gpio_v2_line_info_changed_to_v1(&event, &event_v1);
>                 if (copy_to_user(buf + bytes_read, &event_v1,
>                                                 event_size))
>                                 return -EFAULT;
> }
> 
> if userspace requests a GPIO v1 line info changed event, the kernel populates and returns the event_v1 structure. That structure (https://github.com/torvalds/linux/blob/5bfc75d92efd494db37f5c4c173d3639d4772966/include/uapi/linux/gpio.h#L367) contains 5 words of padding at the end of the structure that do not appear to be initialized in the gpio_v2_line_info_change_to_v1 routine (nor its subordinate routines):
> 
> struct gpioline_info_changed {
>                 struct gpioline_info info;
>                 __u64 timestamp;
>                 __u32 event_type;
>                 __u32 padding[5]; /* for future use */
> };
> 
> It appears that this could be a potential kernel information leak to userspace, and could be fixed by zeroing out the padding field before copying the structure to userspace.
> 

Looks like a bug to me too - well spotted :(.

> We wanted to get your thoughts on if you feel this is actually a bug, or if we overlooked something.
> We're proposing to fix this issue by memsetting the entire structure to zero before calling the conversion routine; if you agree that that's a valid approach, I'm happy to submit an official patch.
> 

Go for it.
I'd zero the padding in the conversion routine myself, but zeroing the
whole struct in the same routine as the copy_to_user(), as you suggest,
would more clearly demonstrate that it isn't leaking stack.

Cheers,
Kent.