From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S932876AbdKGIPw (ORCPT <rfc822;w@1wt.eu>);
        Tue, 7 Nov 2017 03:15:52 -0500
Received: from merlin.infradead.org ([205.233.59.134]:54880 "EHLO
        merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932229AbdKGIPv (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 7 Nov 2017 03:15:51 -0500
Date: Tue, 7 Nov 2017 09:15:41 +0100
From: Peter Zijlstra <peterz@infradead.org>
To: Andi Kleen <ak@linux.intel.com>
Cc: Milind Chabbi <chabbi.milind@gmail.com>, jolsa@redhat.com,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Hari Bathini <hbathini@linux.vnet.ibm.com>,
        Jin Yao <yao.jin@linux.intel.com>, Kan Liang <kan.liang@intel.com>,
        Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf/core: fast breakpoint modification via
 _IOC_MODIFY_BREAKPOINT.
Message-ID: <20171107081541.GF3326@worktop>
References: <1510006156-18988-1-git-send-email-chabbi.milind@gmail.com>
 <20171106231658.GE5320@tassilo.jf.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171106231658.GE5320@tassilo.jf.intel.com>
User-Agent: Mutt/1.5.22.1 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Nov 06, 2017 at 03:16:58PM -0800, Andi Kleen wrote:
> > +static int _perf_event_modify_breakpoint(struct perf_event *bp,
> > +					 struct perf_event_attr *attr)
> > +{
> > +	u64 old_addr = bp->attr.bp_addr;
> > +	u64 old_len = bp->attr.bp_len;
> > +	int old_type = bp->attr.bp_type;
> > +	int err = 0;
> > +
> > +	_perf_event_disable(bp);
> > +
> > +	bp->attr.bp_addr = attr->bp_addr;
> > +	bp->attr.bp_type = attr->bp_type;
> > +	bp->attr.bp_len = attr->bp_len;
> 
> You don't check any of the other fields, so user space is free
> to fill in junk. That means they can never be used for anything.
> It would be better to check at least some of them for being
> zero, and also that the type matches the break point.

Yes, the values should at the very least get the exact same validation
they would get on creating an event with those values.