linux-kernel - Re: [PATCH 4.19 019/101] can: peak_usb: add range checking in decode operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201118220545.GB23840@amd>
Date:   Wed, 18 Nov 2020 23:05:45 +0100
From:   Pavel Machek <pavel@....cz>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Stephane Grosjean <s.grosjean@...k-system.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 4.19 019/101] can: peak_usb: add range checking in decode
 operations

HI!

> From: Dan Carpenter <dan.carpenter@...cle.com>
> 
> [ Upstream commit a6921dd524fe31d1f460c161d3526a407533b6db ]
> 
> These values come from skb->data so Smatch considers them untrusted.  I
> believe Smatch is correct but I don't have a way to test this.
> 
> The usb_if->dev[] array has 2 elements but the index is in the 0-15
> range without checks.  The cfd->len can be up to 255 but the maximum
> valid size is CANFD_MAX_DLEN (64) so that could lead to memory
> corruption.

If this is untrusted, does it need to use _nospec() variants?

> index 41988358f63c8..19600d35aac55 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -476,12 +476,18 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
>  				     struct pucan_msg *rx_msg)
>  {
...
>  	const u16 rx_msg_flags = le16_to_cpu(rm->flags);
>  
> +	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
> +		return -ENOMEM;

Furthermore, should it use -EINVAL here

> +	if (pucan_stmsg_get_channel(sm) >= ARRAY_SIZE(usb_if->dev))
> +		return -ENOMEM;

and here, and perhaps use a helper function?

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)