by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 4.19 000/125] 4.19.84-stable review

On Mon, Nov 11, 2019 at 07:27:19PM +0100, Greg Kroah-Hartman wrote:
> This is the start of the stable review cycle for the 4.19.84 release.
> There are 125 patches in this series, all will be posted as a response
> to this one. If anyone has any issues with these being applied, please
> let me know.
>
> Responses should be made by Wed, 13 Nov 2019 18:08:44 +0000.
> Anything received after that time might be too late.
>

Build results:
total: 156 pass: 156 fail: 0
Qemu test results:
total: 390 pass: 390 fail: 0

Guenter

2019-11-13 10:23:02

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH 4.19 062/125] ALSA: usb-audio: Fix possible NULL dereference at create_yamaha_midi_quirk()

Hi!

> From: Takashi Iwai <[email protected]>
>
> commit 60849562a5db4a1eee2160167e4dce4590d3eafe upstream.
>
> The previous addition of descriptor validation may lead to a NULL
> dereference at create_yamaha_midi_quirk() when either injd or outjd is
> NULL. Add proper non-NULL checks.

This is wrong.

> @@ -259,8 +259,8 @@ static int create_yamaha_midi_quirk(stru
> NULL, USB_MS_MIDI_OUT_JACK);
> if (!injd && !outjd)
> return -ENODEV;

Clearly code wants to allow at most one of them to be NULL.

> - if (!snd_usb_validate_midi_desc(injd) ||
> - !snd_usb_validate_midi_desc(outjd))
> + if (!(injd && snd_usb_validate_midi_desc(injd)) ||
> + !(outjd && snd_usb_validate_midi_desc(outjd)))
> return -ENODEV;

Yet it will return here if it is. Correct check would be

+ if (!(!injd || snd_usb_validate_midi_desc(injd)) ||
+ !(!outjd || snd_usb_validate_midi_desc(outjd)))

AFAICT.

Best regards,
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (1.09 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2019-11-13 10:38:36

On Mon 2019-11-11 19:29:22, Greg Kroah-Hartman wrote:
> From: Jean-Baptiste Maneyrol <[email protected]>
>
> [ Upstream commit 6e82ae6b8d11b948b74e71396efd8e074c415f44 ]
>
> Some chips have a fifo overflow bit issue where the bit is always
> set. The result is that every data is dropped.
>
> Change fifo overflow management by checking fifo count against
> a maximum value.
>
> Add fifo size in chip hardware set of values.

> @@ -216,6 +213,18 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> if (result)
> goto end_session;
> fifo_count = get_unaligned_be16(&data[0]);
> +
> + /*
> + * Handle fifo overflow by resetting fifo.
> + * Reset if there is only 3 data set free remaining to mitigate
> + * possible delay between reading fifo count and fifo data.
> + */
> + nb = 3 * bytes_per_datum;
> + if (fifo_count >= st->hw->fifo_size - nb) {
> + dev_warn(regmap_get_device(st->map), "fifo overflow reset\n");
> + goto flush_fifo;
> + }
> +
> /* compute and process all complete datum */
> nb = fifo_count / bytes_per_datum;
> inv_mpu6050_update_period(st, pf->timestamp, nb);

Would this make sense to reduce code duplication?

Signed-off-by: Pavel Machek <[email protected]>

Best regards,
Pavel

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 548e042f7b5b..9b9d90f41610 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -232,16 +232,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
timestamp = inv_mpu6050_get_timestamp(st);
iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
}
-
-end_session:
- mutex_unlock(&st->lock);
- iio_trigger_notify_done(indio_dev->trig);
-
- return IRQ_HANDLED;
+ goto end_session;

flush_fifo:
/* Flush HW and SW FIFOs. */
inv_reset_fifo(indio_dev);
+end_session:
mutex_unlock(&st->lock);
iio_trigger_notify_done(indio_dev->trig);

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Attachments:

(No filename) (2.13 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2019-11-13 12:06:12

by Takashi Iwai

[permalink] [raw]

Subject: Re: [PATCH 4.19 062/125] ALSA: usb-audio: Fix possible NULL dereference at create_yamaha_midi_quirk()

On Wed, 13 Nov 2019 11:19:54 +0100,
Pavel Machek wrote:
>
> Hi!
>
> > From: Takashi Iwai <[email protected]>
> >
> > commit 60849562a5db4a1eee2160167e4dce4590d3eafe upstream.
> >
> > The previous addition of descriptor validation may lead to a NULL
> > dereference at create_yamaha_midi_quirk() when either injd or outjd is
> > NULL. Add proper non-NULL checks.
>
> This is wrong.
>
> > @@ -259,8 +259,8 @@ static int create_yamaha_midi_quirk(stru
> > NULL, USB_MS_MIDI_OUT_JACK);
> > if (!injd && !outjd)
> > return -ENODEV;
>
> Clearly code wants to allow at most one of them to be NULL.
>
> > - if (!snd_usb_validate_midi_desc(injd) ||
> > - !snd_usb_validate_midi_desc(outjd))
> > + if (!(injd && snd_usb_validate_midi_desc(injd)) ||
> > + !(outjd && snd_usb_validate_midi_desc(outjd)))
> > return -ENODEV;
>
> Yet it will return here if it is. Correct check would be
>
> + if (!(!injd || snd_usb_validate_midi_desc(injd)) ||
> + !(!outjd || snd_usb_validate_midi_desc(outjd)))
>
> AFAICT.

Erm, right, but a better representation is:

if ((injd && !snd_usb_validate_midi_desc(injd)) ||
(outjd && !snd_usb_validate_midi_desc(injd)))

thanks,

Takashi

2019-11-13 13:06:40

by Liang, Kan

[permalink] [raw]

Subject: Re: [PATCH 4.19 097/125] perf/x86/uncore: Fix event group support

On 11/13/2019 5:35 AM, Pavel Machek wrote:
> Hi!
>
>> return ret;
>> }
>>
>> +static void uncore_pmu_enable(struct pmu *pmu)
>> +{
>> + struct intel_uncore_pmu *uncore_pmu;
>> + struct intel_uncore_box *box;
>> +
>> + uncore_pmu = container_of(pmu, struct intel_uncore_pmu, pmu);
>> + if (!uncore_pmu)
>> + return;
>
> AFAICT this test can never trigger.
>
>> +static void uncore_pmu_disable(struct pmu *pmu)
>> +{
>> + struct intel_uncore_pmu *uncore_pmu;
>> + struct intel_uncore_box *box;
>> +
>> + uncore_pmu = container_of(pmu, struct intel_uncore_pmu, pmu);
>> + if (!uncore_pmu)
>> + return;
>
> And neither can this one.

I will submit a patch to fix it.

Thanks,
Kan

2019-11-13 13:50:43

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH 4.19 093/125] usb: gadget: configfs: fix concurrent issue between composite APIs

On Mon 2019-11-11 19:28:52, Greg Kroah-Hartman wrote:
> From: Peter Chen <[email protected]>
>
> [ Upstream commit 1a1c851bbd706ea9f3a9756c2d3db28523506d3b ]
>
> We meet several NULL pointer issues if configfs_composite_unbind
> and composite_setup (or composite_disconnect) are running together.
> These issues occur when do the function switch stress test, the
> configfs_compsoite_unbind is called from user mode by
> echo "" to /sys/../UDC entry, and meanwhile, the setup interrupt
> or disconnect interrupt occurs by hardware. The composite_setup

> +++ b/drivers/usb/gadget/configfs.c
> @@ -61,6 +61,8 @@ struct gadget_info {
> bool use_os_desc;
> char b_vendor_code;
> char qw_sign[OS_STRING_QW_SIGN_LEN];
> + spinlock_t spinlock;
> + bool unbind;
> };
>
> static inline struct gadget_info *to_gadget_info(struct config_item *item)
> @@ -1244,6 +1246,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
> int ret;
>
> /* the gi->lock is hold by the caller */
> + gi->unbind = 0;
> cdev->gadget = gadget;

Since variable is bool, I'd expect "= false" here?

> + unsigned long flags;
>
> /* the gi->lock is hold by the caller */

"is held".

> cdev = get_gadget_data(gadget);
> gi = container_of(cdev, struct gadget_info, cdev);
> + spin_lock_irqsave(&gi->spinlock, flags);
> + gi->unbind = 1;

= true;

> +static int configfs_composite_setup(struct usb_gadget *gadget,
> + const struct usb_ctrlrequest *ctrl)
> +{
> + struct usb_composite_dev *cdev;
> + struct gadget_info *gi;
> + unsigned long flags;
> + int ret;
> +
> + cdev = get_gadget_data(gadget);
> + if (!cdev)
> + return 0;
> +
> + gi = container_of(cdev, struct gadget_info, cdev);
> + spin_lock_irqsave(&gi->spinlock, flags);
> + cdev = get_gadget_data(gadget);

cdev already contains required value, why get it second time? (If it
needs to be done under lock, comment might be useful...)

Best regards,
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (2.09 kB)
signature.asc (201.00 B)
Download all attachments

2019-11-13 15:56:34

Hi!

> > > Clearly code wants to allow at most one of them to be NULL.
> > >
> > > > - if (!snd_usb_validate_midi_desc(injd) ||
> > > > - !snd_usb_validate_midi_desc(outjd))
> > > > + if (!(injd && snd_usb_validate_midi_desc(injd)) ||
> > > > + !(outjd && snd_usb_validate_midi_desc(outjd)))
> > > > return -ENODEV;
> > >
> > > Yet it will return here if it is. Correct check would be
> > >
> > > + if (!(!injd || snd_usb_validate_midi_desc(injd)) ||
> > > + !(!outjd || snd_usb_validate_midi_desc(outjd)))
> > >
> > > AFAICT.
> >
> > Erm, right, but a better representation is:
> >
> > if ((injd && !snd_usb_validate_midi_desc(injd)) ||
> > (outjd && !snd_usb_validate_midi_desc(injd)))
>
> Of course, another typo:
>
> if ((injd && !snd_usb_validate_midi_desc(injd)) ||
> (outjd && !snd_usb_validate_midi_desc(outjd)))

Agreed, that is better. I was focusing on boolean algebra too much.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Attachments:

(No filename) (1.12 kB)
signature.asc (201.00 B)
Download all attachments