From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757309AbcIPFu1 (ORCPT ); Fri, 16 Sep 2016 01:50:27 -0400 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:55410 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754064AbcIPFuT (ORCPT ); Fri, 16 Sep 2016 01:50:19 -0400 Date: Fri, 16 Sep 2016 07:50:15 +0200 From: Pavel Machek To: David Lechner Cc: Jacek Anaszewski , Richard Purdie , linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org, Marcel Holtmann Subject: Re: [PATCH v3] leds: Introduce userspace leds driver Message-ID: <20160916055014.GA13205@amd> References: <1473439776-15655-1-git-send-email-david@lechnology.com> <80597ded-f4b4-2990-3eae-e72276296d1a@samsung.com> <20160915130831.GJ13132@amd> <313cbae5-fd66-f0ae-79a9-a3f4273d6f9c@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi! > >>>>+ if (copy_from_user(&udev->user_dev, buffer, > >>>>+ sizeof(struct uleds_user_dev))) { > >>>>+ ret = -EFAULT; > >>>>+ goto out; > >>>>+ } > >>>>+ > >>>>+ if (!udev->user_dev.name[0]) { > >>>>+ ret = -EINVAL; > >>>>+ goto out; > >>>>+ } > >>>>+ > >>>>+ ret = led_classdev_register(NULL, &udev->led_cdev); > >>>>+ if (ret < 0) > >>>>+ goto out; > >> > >>No sanity checking on the name -> probably a security hole. Do not > >>push this upstream before this is fixed. > > > > If this is a serious security issue, then you should also raise an issue > with input maintainers because this is the extent of sanity checking for > uinput device names as well. I guess that should be fixed. But lets not add new ones. > I must confess that I am no security expert, so unless you can give specific > examples of what potential threats are, I will not be able to guess what I > need to do to fix it. > > After some digging around the kernel, I don't see many instances of > validating device node names. The best I have found so far comes from > create_entry() in binfmt_misc.c > > if (!e->name[0] || > !strcmp(e->name, ".") || > !strcmp(e->name, "..") || > strchr(e->name, '/')) > goto einval; > > Would something like this be a sufficient sanity check? I suppose we could > also check for non-printing characters, but I don't think ignoring them > would be a security issue. That would be minimum, yes. I guess it would be better/easier to just limit the names to [a-zA-Z:-_0-9]*? Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html