Re: [PATCH v4] USB: serial: garmin_gps: validate packet data length in nat_receive()

[Johan Hovold <johan@kernel.org>]

On Wed, Sep 30, 2026 at 07:52:47AM +0000, Hui Peng wrote:
> In nat_receive() (called from garmin_write() when userspace writes to
> /dev/ttyUSBn in MODE_NATIVE mode), while garmin_data_p->insize is less
> than GARMIN_PKTHDR_LENGTH (12), the loop copies up to 12 bytes into
> garmin_data_p->inbuffer. Once garmin_data_p->insize reaches
> GARMIN_PKTHDR_LENGTH, nat_receive() computes the total packet size as:
> 
>   len = GARMIN_PKTHDR_LENGTH + getDataLength(garmin_data_p->inbuffer);
> 
> Because getDataLength() returns a signed int from __le32_to_cpup(), a
> 12-byte header with a negative 32-bit length field (such as 0xfffffff5,
> i.e., -11) causes len to underflow to a small positive value (12 + (-11)
> = 1) without any bounds check at that point. Then garmin_data_p->insize
> >= len (12 >= 1) evaluates to true and nat_receive() calls
> garmin_write_bulk(port, inbuffer, 1, 0), which allocates a 1-byte slab
> buffer via kmemdup() and immediately reads 4 bytes from it in
> getLayerId(buffer) (and again in garmin_write_bulk_callback() when the
> URB completes):

> Validate the unsigned 32-bit data length (dlen >= GPS_IN_BUFSIZ -
> GARMIN_PKTHDR_LENGTH, matching the existing len >= GPS_IN_BUFSIZ bound)
> in nat_receive() as soon as the 12-byte header is present, resetting
> insize and returning -EINVPKT on invalid lengths so garmin_write_bulk()
> is only ever called with a full valid packet header (12 <= len <
> GPS_IN_BUFSIZ).

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
> Changes in v4:
> - Kept the fix strictly inside nat_receive() and dropped the other hunks
>   from v3 (garmin_write_bulk_callback(), gsp_send(), and changing the
>   return type of getDataLength() / getPacketId()) per Greg
>   Kroah-Hartman:
>   1. Assigning u32 dlen = getDataLength(garmin_data_p->inbuffer) locally
>      in nat_receive() and rejecting dlen >= GPS_IN_BUFSIZ -
>      GARMIN_PKTHDR_LENGTH (matching the existing len >= GPS_IN_BUFSIZ
>      check at the top of the loop) prevents both negative/underflowed
>      lengths (< 12 bytes) and oversized lengths without needing to touch
>      getDataLength() across the file.
>   2. The out-of-bounds read in garmin_write_bulk() and
>      garmin_write_bulk_callback() was only reachable because
>      nat_receive() allowed len to underflow below GARMIN_PKTHDR_LENGTH
>      (12 bytes). All other callers of garmin_write_bulk() always pass at
>      least GARMIN_PKTHDR_LENGTH bytes, making a separate length check in
>      garmin_write_bulk_callback() redundant.
>   3. In MODE_GARMIN_SERIAL, gsp_receive() already bounds insize to
>      MAX_SERIAL_PKT_SIZ + 2 before calling gsp_send(), so the extra
>      bounds check in gsp_send() was an unrelated defensive check.
> - Added Cc: stable@vger.kernel.org and QEMU reproduction details.
> 
>  drivers/usb/serial/garmin_gps.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/garmin_gps.c b/drivers/usb/serial/garmin_gps.c
> index 8020149f5658..a3f599043a4d 100644
> --- a/drivers/usb/serial/garmin_gps.c
> +++ b/drivers/usb/serial/garmin_gps.c
> @@ -769,8 +769,14 @@ static int nat_receive(struct garmin_data *garmin_data_p,
>  
>  		/* do we have a complete packet ? */
>  		if (garmin_data_p->insize >= GARMIN_PKTHDR_LENGTH) {
> -			len = GARMIN_PKTHDR_LENGTH+
> -			   getDataLength(garmin_data_p->inbuffer);
> +			u32 dlen = getDataLength(garmin_data_p->inbuffer);
> +
> +			if (dlen >= GPS_IN_BUFSIZ - GARMIN_PKTHDR_LENGTH) {
> +				garmin_data_p->insize = 0;
> +				result = -EINVPKT;
> +				break;
> +			}
> +			len = GARMIN_PKTHDR_LENGTH + dlen;

Shouldn't you fix the sanity check at the top of the loop instead so
that we never reach this conditional in case of a malformed header?

>  			if (garmin_data_p->insize >= len) {
>  				garmin_write_bulk(garmin_data_p->port,
>  						   garmin_data_p->inbuffer,

Johan