Date:   Wed, 8 Mar 2023 12:20:24 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
CC:     "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Len Brown <lenb@kernel.org>,
        Robert Moore <robert.moore@intel.com>,
        <linux-acpi@vger.kernel.org>,
        <acpica-devel@lists.linuxfoundation.org>,
        <linux-kernel@vger.kernel.org>,
        Hariganesh Govindarajulu <hariganesh.govindarajulu@intel.com>
Subject: Re: [PATCH] ACPI: tools: pfrut: Check if the input of level and type
 is in the right numeric range
Message-ID: <ZAgNCCUMjP3W35qd@chenyu5-mobl1>
References: <20230307114145.8933-1-yu.c.chen@intel.com>
 <CAJZ5v0h8Z8_zb_seSfsFsgWksG6N41jFcuTycrEicJYJOG1yLQ@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJZ5v0h8Z8_zb_seSfsFsgWksG6N41jFcuTycrEicJYJOG1yLQ@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?djdEZURMbEFDaStxQ3ptWGR2dmhNeWxUcjVnZTYzMWlhbHdKeW5tQlhjQ2hl?=
 =?utf-8?B?L1JuU3NEa2R1TDdmdTRKbHYzMjJlcThnQlFFNGhTNklidDN2R2JxWVNIeFNS?=
 =?utf-8?B?V2NFNWhaMUc5eVlIeHRFMmt0R09ObEd5V21yV3FBTEcycXJlT1VXUUNKSmhi?=
 =?utf-8?B?WUYyOHA2cUpKYlREUUN3R0lkTlBZTS9nclV6K2htWXdPbDNNM1lpZHI1VFpK?=
 =?utf-8?B?R3RMc1NHbGpQN3JYVjlzMXM2SWVOdEs0V2dVWlAyV3F2MXdEeUtqTWpNeE1x?=
 =?utf-8?B?eHlKSndPR2dMR2ZPRDVFdTlnd0ppV25ZcUkzUUZjNFhNZ2JZVk5XeVBUZU1J?=
 =?utf-8?B?KzFmVWh4cy8wNEtxVFoxbmdKYjU2cmdpWGxqM1IvQjJOZlExU0ptbGlXTU1K?=
 =?utf-8?B?SzZXcGplN1p0WWNscXBMdENTY1Q2WjU0UlhFSHFNN3RyTWV5MHU2a2xrK1NU?=
 =?utf-8?B?OTNiUnp5UHZ2UElyaUowZUc1bTZmTFoveUdGTUhRNjdhZElQcmc5cHZRZUIr?=
 =?utf-8?B?WDl3LzJmaHdZZGJpUHYvb3E3OVNkbWV1N1Juamx1L1N5T0Y0eU05UHFKcnpE?=
 =?utf-8?B?YjltWVovSmVwcDBEdE1RaFVjK1U2RzhiaVBGSEl4elNERk9UMDlSc2g1Wmw1?=
 =?utf-8?B?N1pteS9aenJDUXZxRVlXNHRBZWF5VGwwOHNhSGNBbjlFWE13Z25mTExpS2RJ?=
 =?utf-8?B?bDNCT3JtSTJmZXVQWVBPUHVjSkk0MXBHTVY1OThuemhNdmV1UmpJU2UvM0Q1?=
 =?utf-8?B?M2JEREU2MHMxMEsydWM4aTBtN0Y5U1UrZlRHY1dzeitnRnRJOUZDOEFXbG9h?=
 =?utf-8?B?UGFrZ1RiUko4eWlsdzV0d2VVcitlYVNUNHpYRzJhWDgyMXlrNk9lU2g3a1Vr?=
 =?utf-8?B?OGJUakpEV0JDanY5REpEQVpnZVpZZDVzNWFLMWhJRzNZTHFWSW5SQkhBU05P?=
 =?utf-8?B?cDc3ZCtJOUF5V2paK3dTVHFmZzQ3K1NkWHVUQ1ZVWUozdjYrN2toZm14dkc5?=
 =?utf-8?B?OEFyeUFoZFpjYnN1UDN6QzdPaEpDMk43RFJuVlROUUlEbzIvcExIbHl0dk04?=
 =?utf-8?B?Y1R5U1N6ZXB3b0hPaVlTYks5REtyM0JSaUdBd0U3anhUb1NRczgvakpvTHVl?=
 =?utf-8?B?Ynl6VzFVT3YyVkQ1UEZVR2FnR3N2NTh6OXRjNXcwK1g2UXJzRWFFTFhWMUY3?=
 =?utf-8?B?WjZpOWZ1VFEvb2FOS2w2Vm40c0g2S3psZkdRblBneVg5YkpnRG92bVRwamIv?=
 =?utf-8?B?aEMvMEROZWFrQnRBQi9GRzZNZzF5bXkvRDlkYURobW55ZnpIOWI4T3Vjd1pZ?=
 =?utf-8?B?dGVQV1h1WlovTkx3Z2diWlpaaTVmZitDTzF4QjhIK3pwNlZ0SkhGcS9rLzZk?=
 =?utf-8?B?QXBLR2ZvMGZ3U0JpUHh4TWVkbFVYR2cxejI1elhramNxWXVWd084bkdKQy8v?=
 =?utf-8?B?VTFFUW94aTdwZVhoTlBTZmNEWXU5bkVUZGNwK3pZZWl3c1JHQ2tlMTV2QmRp?=
 =?utf-8?B?OWd2WTN5aXYwdzVkdFhJbHgvV0wrZndadE9GOW9KeTAyVE1kcXViQVhMM2g0?=
 =?utf-8?B?TTh2SXpWRGw2NVAzTEQ4RCthYXdlR0RPdlR1VFdTSkt0MmxPYVZDdWppeUJT?=
 =?utf-8?B?ZVlEVkd2Q1hKc0kweFJRK2dkUW5sdDJldTQ1VCs5UWJyMlBFZWtFN0VLVGVh?=
 =?utf-8?B?Zy9LcmgzbnlXVjhuYi9MQ05rTjh2RHpSdHhtZWdxMUZHZUlIWEV6RHRXWERJ?=
 =?utf-8?B?MjFjaHNoOHZnVmRYWFpxOVUzQngvS2hkSjdNOXBWaXJxWDExdFlWUE5kRFFn?=
 =?utf-8?B?NUZoYWJGL0ZSanNEMUJ1NWVJNiswMlhxU1FWOTdMbzBERFZQa0tyQTJGVUxq?=
 =?utf-8?B?SHg5U3BmUGorRDBGeDk1UlQzdm85ZFd2eit3WHJPWDZseEJkTnZVUlNxV1Iw?=
 =?utf-8?B?cW5tZU5kUmRqWmhNNEhXTEN5MGZOZG9aelJyOUVqU0pBcHJSOEVVL3hTV3dT?=
 =?utf-8?B?OXZmcDcxbnpXRDVLVmlkSDRMWHlJV2pPWGJxQmNGZE9WSmppOHNOQUNiYVdy?=
 =?utf-8?B?MGpkOUlic1MxVkhuSzhVNHZxT2Q3YzhjT2RrYStsYktkdHFBeVhBeU1FOEZH?=
 =?utf-8?Q?WronzscZC1j+tIXu5cxvxIBWM?=
X-MS-Exchange-CrossTenant-Network-Message-Id: de2ddf9e-0f50-456f-db51-08db1f8c788b
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2023 04:20:38.3570
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: MFger9QQLeXgjD4Ffq17306JT/hcZixTKhAQDDRPGh7wezr1hyfotu8AHFQ5qyKIpaFmvQ6oEKm4oxBTFXUN9w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7003
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Rafael,
thanks for the review,
On 2023-03-07 at 12:38:02 +0100, Rafael J. Wysocki wrote:
> On Tue, Mar 7, 2023 at 4:46 AM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > The user can provide arbitrary non-numeic value to level and type, which
> > brings unexpected behavior:
> 
> So I guess the expected behavior would be to throw an error?
> 
Yes.
> >  pfrut -h
> > usage: pfrut [OPTIONS]
> > code injection:
> > -l, --load
> > -s, --stage
> > -a, --activate
> > -u, --update [stage and activate]
> > -q, --query
> > -d, --revid
> > update telemetry:
> > -G, --getloginfo
> > -T, --type(0:execution, 1:history)
> > -L, --level(0, 1, 2, 4)
> > -R, --read
> > -D, --revid log
> >
> >  pfrut -T A
> >  pfrut -G
> > log_level:0
> > log_type:0
> > log_revid:2
> > max_data_size:65536
> > chunk1_size:0
> > chunk2_size:1530
> > rollover_cnt:0
> > reset_cnt:17
> >
> > Fix this by restricting the input is in the expected range.
> >
> > Reported-by: Hariganesh Govindarajulu <hariganesh.govindarajulu@intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  tools/power/acpi/tools/pfrut/pfrut.c | 33 ++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/tools/power/acpi/tools/pfrut/pfrut.c b/tools/power/acpi/tools/pfrut/pfrut.c
> > index 52aa0351533c..ed672efef83b 100644
> > --- a/tools/power/acpi/tools/pfrut/pfrut.c
> > +++ b/tools/power/acpi/tools/pfrut/pfrut.c
> > @@ -22,6 +22,7 @@
> >  #include <sys/ioctl.h>
> >  #include <sys/mman.h>
> >  #include <uuid/uuid.h>
> > +#include <ctype.h>
> >  #include PFRUT_HEADER
> >
> >  char *capsule_name;
> > @@ -77,6 +78,18 @@ static void help(void)
> >                 progname);
> >  }
> >
> > +static int is_digit_input(char *str)
> > +{
> > +       char *scan;
> > +
> > +       for (scan = str; *scan != '\0'; scan++) {
> > +               if (!isdigit(*scan))
> > +                       return 0;
> > +       }
> > +
> > +       return 1;
> > +}
> > +
> >  char *option_string = "l:sauqd:GT:L:RD:h";
> >  static struct option long_options[] = {
> >         {"load", required_argument, 0, 'l'},
> > @@ -125,11 +138,31 @@ static void parse_options(int argc, char **argv)
> >                         log_getinfo = 1;
> >                         break;
> >                 case 'T':
> > +                       if (!is_digit_input(optarg)) {
> > +                               printf("Please provide numeric value for type (0:execution, 1:history) - Exiting.\n");
> > +                               exit(1);
> > +                       }
> > +
> 
> Wouldn't using strtol() instead of atoi() work?
> 
Yes, strtol() would be simpler:
c = strtol(optarg, &endptr, 0)
if (*endptr)
	exit(1);
I'll change it in next version.

thanks,
Chenyu